[sr-dev] git:master:74c84c7c: db_postgres: Fix heap use after free error in db_postgres module

[Daniel-Constantin Mierla]

Hello,

any follow up on this one? It would be good to sort it out before next
minor release.

Cheers,
Daniel

On 04/09/15 12:21, Daniel-Constantin Mierla wrote:
> One more thing:
>
> some code related to a report on bug tracker was removed:
>
> -	memset(keywords, 0, (sizeof(char*) * 10));
> -	memset(values, 0, (sizeof(char*) * 10));
> -	memset(to, 0, (sizeof(char) * 16));
>
>
> Was this intentional and related to your case? It is not related to
> columns from pg result.
>
> Cheers,
> Daniel
>
> On 04/09/15 12:07, Daniel-Constantin Mierla wrote:
>> Hello,
>>
>> following the discussion started by Chris Double, I am not sure this is
>> the right fix. The column names are now allocated in pkg, but don't get
>> freed, so looks like this commit introduced a memory leak. See:
>>
>> http://lists.sip-router.org/pipermail/sr-dev/2015-September/030565.html
>>
>> I think the commit has to be reverted and then remove
>> db_postgres_free_query() at the end of db_postgres_store_result()
>>
>> Not using postgres module, but this seems the right from c coding point
>> of view comparing with db_mysql.
>>
>> Cheers,
>> Daniel
>>
>> On 28/08/15 12:11, Carsten Bock wrote:
>>> Module: kamailio
>>> Branch: master
>>> Commit: 74c84c7cd52347fcd1c90e75dca239b5f758169b
>>> URL: https://github.com/kamailio/kamailio/commit/74c84c7cd52347fcd1c90e75dca239b5f758169b
>>>
>>> Author: Carsten Bock <carsten at ng-voice.com>
>>> Committer: Carsten Bock <carsten at ng-voice.com>
>>> Date: 2015-08-28T12:11:25+02:00
>>>
>>> db_postgres: Fix heap use after free error in db_postgres module
>>>
>>> The result structure for a query holds a pointer returned by
>>> PQfname. When sql_do_query executes the query it gets this
>>> database result structure returned but the PQfname pointer
>>> has already been free'd by a call to db_postgres_free_query
>>> from within db_postgres_store_result.
>>>
>>> sql_do_query then tries to copy the free'd string into another
>>> result structure resulting in a heap use after free.
>>>
>>> The fix here copies the PQfname result.
>>>
>>> Fix by Chris Double
>>>
>>> ---
>>>
>>> Modified: modules/db_postgres/km_pg_con.c
>>> Modified: modules/db_postgres/km_res.c
>>>
>>> ---
>>>
>>> Diff:  https://github.com/kamailio/kamailio/commit/74c84c7cd52347fcd1c90e75dca239b5f758169b.diff
>>> Patch: https://github.com/kamailio/kamailio/commit/74c84c7cd52347fcd1c90e75dca239b5f758169b.patch
>>>
>>> ---
>>>
>>> diff --git a/modules/db_postgres/km_pg_con.c b/modules/db_postgres/km_pg_con.c
>>> index ec98add..d053c55 100644
>>> --- a/modules/db_postgres/km_pg_con.c
>>> +++ b/modules/db_postgres/km_pg_con.c
>>> @@ -71,10 +71,6 @@ struct pg_con* db_postgres_new_connection(struct db_id* id)
>>>  	memset(ptr, 0, sizeof(struct pg_con));
>>>  	ptr->ref = 1;
>>>  
>>> -	memset(keywords, 0, (sizeof(char*) * 10));
>>> -	memset(values, 0, (sizeof(char*) * 10));
>>> -	memset(to, 0, (sizeof(char) * 16));
>>> -
>>>  	if (id->port) {
>>>  		ports = int2str(id->port, 0);
>>>  		keywords[i] = "port";
>>> diff --git a/modules/db_postgres/km_res.c b/modules/db_postgres/km_res.c
>>> index e9aa232..912206b 100644
>>> --- a/modules/db_postgres/km_res.c
>>> +++ b/modules/db_postgres/km_res.c
>>> @@ -126,8 +126,14 @@ int db_postgres_get_columns(const db1_con_t* _h, db1_res_t* _r)
>>>  				RES_NAMES(_r)[col]);
>>>  
>>>  		/* The pointer that is here returned is part of the result structure. */
>>> -		RES_NAMES(_r)[col]->s = PQfname(CON_RESULT(_h), col);
>>> -		RES_NAMES(_r)[col]->len = strlen(PQfname(CON_RESULT(_h), col));
>>> +		RES_NAMES(_r)[col]->s = pkg_malloc(strlen(PQfname(CON_RESULT(_h), col))+1);
>>> +		if (! RES_NAMES(_r)[col]->s) {
>>> +			LM_ERR("no private memory left\n");
>>> +			db_free_columns(_r);
>>> +			return -4;
>>> +		}
>>> +		strcpy(RES_NAMES(_r)[col]->s, PQfname(CON_RESULT(_h), col));
>>> +		RES_NAMES(_r)[col]->len = strlen(RES_NAMES(_r)[col]->s);
>>>  
>>>  		LM_DBG("RES_NAMES(%p)[%d]=[%.*s]\n", RES_NAMES(_r)[col], col,
>>>  				RES_NAMES(_r)[col]->len, RES_NAMES(_r)[col]->s);
>>>
>>>
>>> _______________________________________________
>>> sr-dev mailing list
>>> sr-dev at lists.sip-router.org
>>> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

-- 
Daniel-Constantin Mierla
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Book: SIP Routing With Kamailio - http://www.asipto.com
Kamailio Advanced Training, Sep 28-30, 2015, in Berlin - http://asipto.com/u/kat