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?
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?
-
Chris Double -
Daniel-Constantin Mierla