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

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/74c84c7cd52347fcd1c90e75dca239b… >> >> Author: Carsten Bock <carsten(a)ng-voice.com> >> Committer: Carsten Bock <carsten(a)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/74c84c7cd52347fcd1c90e75dca239b… >> Patch: https://github.com/kamailio/kamailio/commit/74c84c7cd52347fcd1c90e75dca239b… >> >> --- >> >> 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(a)lists.sip-router.org >> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev