[sr-dev] db_free_columns and freeing column name

[Daniel-Constantin Mierla]

Hello,

db_mysql keeps the original mysql result as long as db1_res_t result is
available (one of the reasons being also the db-fetch capability, along
with eventual references to the mysql result structure).

Looking at the code, I think the db_postgress is wrong calling
db_postgres_free_query() at the end of db_postgres_store_result().

db_postgres_free_query() is executed from db_postgres_free_result(),
which is called when the db1_res_t is freed by the module using the result.

On another hand, for values in db rows of db1_res_t, there should be a
flag that specifies if the value needs to be freed or not. In case
column names need to be allocated, such flag should be used there as
well, so the function in the lib knows whether to do free or not for
column name.

Cheers,
Daniel

On 30/08/15 12:26, Chris Double wrote:
> This question relates to the commit in
> http://lists.sip-router.org/pipermail/sr-dev/2015-August/030514.html
>
> The db1_res_1 structure stores a string for the column names in the
> query. The commit in the email referenced above fixed an issue in the
> postgres backend where it was storing an internal postgres pointer in
> the result structure but by the time the results were used in sqlops a
> PQclear had been performed leaving that pointer dangling. Eventually a
> use after free occurs.
>
> The fix in the commit is to copy the column name into a pkg_malloc'd
> area. The question I have is where should this be free'd.  I thought
> db_free_columns in lib/srdb1/db_res.c would be the place. In that
> function it frees the column str object (RES_NAMES(_r)[col]) but not
> the string char* (RES_NAMES(_r)[col]->s).
>
> Changing that function to free the string would seem to be the right
> fix - is it free'd anywhere else that I'm missing?
>
> Looking at the other database backends to work out whether they store
> anything there that needs to be free'd shows similar issues to the
> postgres one that was fixed in the commit referenced earlier:
>
> db_berkeley: Uses a pointer to internal database results
> db_mongodb: Uses a pointer to internal database results
> db_mysql: Uses a pointer to internal database results
> db_text: Uses a pointer to internal database results
> db_unixodbc: Uses a pointer to a stack variable
>
> The other backends (oracle, sqlite) seem to do the same as the fix
> made to the postgres backend.
>
> The unixodbc backend seem to have a dangling pointer into the stack
> which is problematic. Should these DB backends be changed to copy the
> column name instead of storing an internal pointer?  If so, is
> db_free_columns the correct place to free that memory?
>
> Is there anywhere else that stores column name data in results which
> might need to be modified?
>

-- 
Daniel-Constantin Mierla
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Book: SIP Routing With Kamailio - http://www.asipto.com