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