Module: sip-router Branch: master Commit: 94ce2b1de63432baaaecd9285608380a40a70550 URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=94ce2b1d...
Author: Richard Fuchs rfuchs@sipwise.com Committer: Richard Fuchs rfuchs@sipwise.com Date: Wed Apr 10 09:34:49 2013 -0400
db_mysql: fix segfault when recursive queries are made
The MySQL result object (MYSQL_RES) should not be stored within the srdb1 connection object, but rather within the srdb1 result object. Otherwise recursive queries overwrite each other's result sets, which results in segfault.
---
modules/db_mysql/km_dbase.c | 36 +++++++++++++++++++----------------- modules/db_mysql/km_dbase.h | 2 +- modules/db_mysql/km_my_con.c | 1 - modules/db_mysql/km_my_con.h | 4 ---- modules/db_mysql/km_res.c | 27 +++++++++++++++++++++++---- modules/db_mysql/km_res.h | 19 +++++++++++++++++++ modules/db_mysql/km_row.c | 5 +++-- 7 files changed, 65 insertions(+), 29 deletions(-)
diff --git a/modules/db_mysql/km_dbase.c b/modules/db_mysql/km_dbase.c index 7288b66..4cdcef0 100644 --- a/modules/db_mysql/km_dbase.c +++ b/modules/db_mysql/km_dbase.c @@ -163,14 +163,14 @@ static int db_mysql_store_result(const db1_con_t* _h, db1_res_t** _r) return -1; }
- *_r = db_new_result(); + *_r = db_mysql_new_result(); if (*_r == 0) { LM_ERR("no memory left\n"); return -2; }
- CON_RESULT(_h) = mysql_store_result(CON_CONNECTION(_h)); - if (!CON_RESULT(_h)) { + RES_RESULT(*_r) = mysql_store_result(CON_CONNECTION(_h)); + if (!RES_RESULT(*_r)) { if (mysql_field_count(CON_CONNECTION(_h)) == 0) { (*_r)->col.n = 0; (*_r)->n = 0; @@ -181,7 +181,7 @@ static int db_mysql_store_result(const db1_con_t* _h, db1_res_t** _r) if (code == CR_SERVER_GONE_ERROR || code == CR_SERVER_LOST) { counter_inc(mysql_cnts_h.driver_err); } - db_free_result(*_r); + db_mysql_free_result(_h, *_r); *_r = 0; return -3; } @@ -195,14 +195,14 @@ static int db_mysql_store_result(const db1_con_t* _h, db1_res_t** _r) /* all mem on Kamailio API side is already freed by * db_mysql_convert_result in case of error, but we also need * to free the mem from the mysql lib side */ - mysql_free_result(CON_RESULT(_h)); + mysql_free_result(RES_RESULT(*_r)); #if (MYSQL_VERSION_ID >= 40100) while( mysql_more_results(CON_CONNECTION(_h)) && mysql_next_result(CON_CONNECTION(_h)) > 0 ) { MYSQL_RES *res = mysql_store_result( CON_CONNECTION(_h) ); mysql_free_result(res); } #endif - CON_RESULT(_h) = 0; + RES_RESULT(*_r) = 0; return -4; }
@@ -224,19 +224,21 @@ done: * \param _r result set that should be freed * \return zero on success, negative value on failure */ -int db_mysql_free_result(db1_con_t* _h, db1_res_t* _r) +int db_mysql_free_result(const db1_con_t* _h, db1_res_t* _r) { if ((!_h) || (!_r)) { LM_ERR("invalid parameter value\n"); return -1; }
+ mysql_free_result(RES_RESULT(_r)); + RES_RESULT(_r) = 0; + pkg_free(RES_PTR(_r)); + if (db_free_result(_r) < 0) { LM_ERR("unable to free result structure\n"); return -1; } - mysql_free_result(CON_RESULT(_h)); - CON_RESULT(_h) = 0; return 0; }
@@ -288,21 +290,21 @@ int db_mysql_fetch_result(const db1_con_t* _h, db1_res_t** _r, const int nrows)
/* exit if the fetch count is zero */ if (nrows == 0) { - db_free_result(*_r); + db_mysql_free_result(_h, *_r); *_r = 0; return 0; }
if(*_r==0) { /* Allocate a new result structure */ - *_r = db_new_result(); + *_r = db_mysql_new_result(); if (*_r == 0) { LM_ERR("no memory left\n"); return -2; }
- CON_RESULT(_h) = mysql_store_result(CON_CONNECTION(_h)); - if (!CON_RESULT(_h)) { + RES_RESULT(*_r) = mysql_store_result(CON_CONNECTION(_h)); + if (!RES_RESULT(*_r)) { if (mysql_field_count(CON_CONNECTION(_h)) == 0) { (*_r)->col.n = 0; (*_r)->n = 0; @@ -313,7 +315,7 @@ int db_mysql_fetch_result(const db1_con_t* _h, db1_res_t** _r, const int nrows) if (code == CR_SERVER_GONE_ERROR || code == CR_SERVER_LOST) { counter_inc(mysql_cnts_h.driver_err); } - db_free_result(*_r); + db_mysql_free_result(_h, *_r); *_r = 0; return -3; } @@ -323,7 +325,7 @@ int db_mysql_fetch_result(const db1_con_t* _h, db1_res_t** _r, const int nrows) return -4; }
- RES_NUM_ROWS(*_r) = mysql_num_rows(CON_RESULT(_h)); + RES_NUM_ROWS(*_r) = mysql_num_rows(RES_RESULT(*_r)); if (!RES_NUM_ROWS(*_r)) { LM_DBG("no rows returned from the query\n"); RES_ROWS(*_r) = 0; @@ -362,8 +364,8 @@ int db_mysql_fetch_result(const db1_con_t* _h, db1_res_t** _r, const int nrows) }
for(i = 0; i < rows; i++) { - CON_ROW(_h) = mysql_fetch_row(CON_RESULT(_h)); - if (!CON_ROW(_h)) { + RES_ROW(*_r) = mysql_fetch_row(RES_RESULT(*_r)); + if (!RES_ROW(*_r)) { LM_ERR("driver error: %s\n", mysql_error(CON_CONNECTION(_h))); RES_ROW_N(*_r) = i; db_free_rows(*_r); diff --git a/modules/db_mysql/km_dbase.h b/modules/db_mysql/km_dbase.h index 52b2778..23fc002 100644 --- a/modules/db_mysql/km_dbase.h +++ b/modules/db_mysql/km_dbase.h @@ -58,7 +58,7 @@ void db_mysql_close(db1_con_t* _h); /*! \brief * Free all memory allocated by get_result */ -int db_mysql_free_result(db1_con_t* _h, db1_res_t* _r); +int db_mysql_free_result(const db1_con_t* _h, db1_res_t* _r);
/*! \brief diff --git a/modules/db_mysql/km_my_con.c b/modules/db_mysql/km_my_con.c index ce20be2..646d74b 100644 --- a/modules/db_mysql/km_my_con.c +++ b/modules/db_mysql/km_my_con.c @@ -144,7 +144,6 @@ void db_mysql_free_connection(struct pool_con* con)
_c = (struct my_con*) con;
- if (_c->res) mysql_free_result(_c->res); if (_c->id) free_db_id(_c->id); if (_c->con) { mysql_close(_c->con); diff --git a/modules/db_mysql/km_my_con.h b/modules/db_mysql/km_my_con.h index 33f2128..8e96e20 100644 --- a/modules/db_mysql/km_my_con.h +++ b/modules/db_mysql/km_my_con.h @@ -44,9 +44,7 @@ struct my_con { unsigned int ref; /*!< Reference count */ struct pool_con* next; /*!< Next connection in the pool */
- MYSQL_RES* res; /*!< Actual result */ MYSQL* con; /*!< Connection representation */ - MYSQL_ROW row; /*!< Actual row in the result */ time_t timestamp; /*!< Timestamp of last query */ int transaction; /*!< indicates whether a multi-query transaction is currently open */ }; @@ -55,9 +53,7 @@ struct my_con { /* * Some convenience wrappers */ -#define CON_RESULT(db_con) (((struct my_con*)((db_con)->tail))->res) #define CON_CONNECTION(db_con) (((struct my_con*)((db_con)->tail))->con) -#define CON_ROW(db_con) (((struct my_con*)((db_con)->tail))->row) #define CON_TIMESTAMP(db_con) (((struct my_con*)((db_con)->tail))->timestamp) #define CON_TRANSACTION(db_con) (((struct my_con*)((db_con)->tail))->transaction)
diff --git a/modules/db_mysql/km_res.c b/modules/db_mysql/km_res.c index 10d3ce2..965e6dc 100644 --- a/modules/db_mysql/km_res.c +++ b/modules/db_mysql/km_res.c @@ -73,7 +73,7 @@ int db_mysql_get_columns(const db1_con_t* _h, db1_res_t* _r) return -3; }
- fields = mysql_fetch_fields(CON_RESULT(_h)); + fields = mysql_fetch_fields(RES_RESULT(_r)); for(col = 0; col < RES_COL_N(_r); col++) { RES_NAMES(_r)[col] = (str*)pkg_malloc(sizeof(str)); if (! RES_NAMES(_r)[col]) { @@ -164,7 +164,7 @@ static inline int db_mysql_convert_rows(const db1_con_t* _h, db1_res_t* _r) return -1; }
- RES_ROW_N(_r) = mysql_num_rows(CON_RESULT(_h)); + RES_ROW_N(_r) = mysql_num_rows(RES_RESULT(_r)); if (!RES_ROW_N(_r)) { LM_DBG("no rows returned from the query\n"); RES_ROWS(_r) = 0; @@ -177,8 +177,8 @@ static inline int db_mysql_convert_rows(const db1_con_t* _h, db1_res_t* _r) }
for(row = 0; row < RES_ROW_N(_r); row++) { - CON_ROW(_h) = mysql_fetch_row(CON_RESULT(_h)); - if (!CON_ROW(_h)) { + RES_ROW(_r) = mysql_fetch_row(RES_RESULT(_r)); + if (!RES_ROW(_r)) { LM_ERR("driver error: %s\n", mysql_error(CON_CONNECTION(_h))); RES_ROW_N(_r) = row; db_free_rows(_r); @@ -221,3 +221,22 @@ int db_mysql_convert_result(const db1_con_t* _h, db1_res_t* _r) return 0; }
+ +/*! + * \brief Allocate new result set with private structure + * \return db1_res_t object on success, NULL on failure + */ +db1_res_t* db_mysql_new_result(void) +{ + db1_res_t* obj; + + obj = db_new_result(); + if (!obj) + return NULL; + RES_PTR(obj) = pkg_malloc(sizeof(struct my_res)); + if (!RES_PTR(obj)) { + db_free_result(obj); + return NULL; + } + return obj; +} diff --git a/modules/db_mysql/km_res.h b/modules/db_mysql/km_res.h index 957a961..0b063d8 100644 --- a/modules/db_mysql/km_res.h +++ b/modules/db_mysql/km_res.h @@ -37,6 +37,18 @@ #include "../../lib/srdb1/db_con.h"
+struct my_res { + MYSQL_RES* res; /*!< Actual result */ + MYSQL_ROW row; /*!< Actual row in the result */ +}; + +/* + * Some convenience wrappers + */ +#define RES_RESULT(db_res) (((struct my_res*)((db_res)->ptr))->res) +#define RES_ROW(db_res) (((struct my_res*)((db_res)->ptr))->row) + + /*! * \brief Fill the result structure with data from database * \param _h database connection @@ -54,4 +66,11 @@ int db_mysql_convert_result(const db1_con_t* _h, db1_res_t* _r); */ int db_mysql_get_columns(const db1_con_t* _h, db1_res_t* _r);
+ +/*! + * \brief Allocate new result set with private structure + * \return db1_res_t object on success, NULL on failure + */ +db1_res_t* db_mysql_new_result(void); + #endif diff --git a/modules/db_mysql/km_row.c b/modules/db_mysql/km_row.c index 42f2841..9f2c730 100644 --- a/modules/db_mysql/km_row.c +++ b/modules/db_mysql/km_row.c @@ -36,6 +36,7 @@ #include "km_my_con.h" #include "km_val.h" #include "km_row.h" +#include "km_res.h"
/*! * \brief Convert a row from result into DB API representation @@ -59,11 +60,11 @@ int db_mysql_convert_row(const db1_con_t* _h, db1_res_t* _res, db_row_t* _r) return -2; } - lengths = mysql_fetch_lengths(CON_RESULT(_h)); + lengths = mysql_fetch_lengths(RES_RESULT(_res));
for(i = 0; i < RES_COL_N(_res); i++) { if (db_str2val(RES_TYPES(_res)[i], &(ROW_VALUES(_r)[i]), - ((MYSQL_ROW)CON_ROW(_h))[i], lengths[i], 0) < 0) { + ((MYSQL_ROW)RES_ROW(_res))[i], lengths[i], 0) < 0) { LM_ERR("failed to convert value\n"); LM_DBG("free row at %p\n", _r); db_free_row(_r);
Richard Fuchs writes:
db_mysql: fix segfault when recursive queries are made
richard,
since this sounds pretty severe bug, are you planning at some point to cherry pick the commit also to 4.0?
-- juha
Hi,
On 04/10/13 11:12, Juha Heinanen wrote:
since this sounds pretty severe bug, are you planning at some point to cherry pick the commit also to 4.0?
Yes absolutely, as well as fixes to any other db drivers I'll make. I was wondering if anyone sees any problems with how this is fixed, before I do that.
cheers
richard,
on debian wheezy, i got this after your commit:
CC (cc) [M db_mysql.so] km_db_mysql.o km_db_mysql.c: In function 'db_mysql_bind_api': km_db_mysql.c:110:24: warning: assignment from incompatible pointer type [enabled by default]
-- juha
On 04/10/13 12:02, Juha Heinanen wrote:
richard,
on debian wheezy, i got this after your commit:
CC (cc) [M db_mysql.so] km_db_mysql.o km_db_mysql.c: In function 'db_mysql_bind_api': km_db_mysql.c:110:24: warning: assignment from incompatible pointer type [enabled by default]
This should be fixed by adding the const qualifier to the db_free_result_f prototype (first arg), however this can only be done after all other db drivers have been updated so they don't store the result set objects in the connection objects any more. If this can't be done short term, then we'll need to fix the warning some other way.
cheers
Richard Fuchs writes:
This should be fixed by adding the const qualifier to the db_free_result_f prototype (first arg), however this can only be done after all other db drivers have been updated so they don't store the result set objects in the connection objects any more. If this can't be done short term, then we'll need to fix the warning some other way.
warning is fine for now as long as it is just a warning.
-- juha
Hello,
On 4/10/13 3:36 PM, Richard Fuchs wrote:
-int db_mysql_free_result(db1_con_t* _h, db1_res_t* _r); +int db_mysql_free_result(const db1_con_t* _h, db1_res_t* _r);
the change to the prototype of the above function is now throwing compilation warning:
km_db_mysql.c: In function ‘db_mysql_bind_api’: km_db_mysql.c:109: warning: assignment from incompatible pointer type
Do you need the const specifier? If yes, the DB API and probably the other database drivers need to be updated.
Cheers, Daniel
Hi,
On 04/29/13 07:11, Daniel-Constantin Mierla wrote:
Hello,
On 4/10/13 3:36 PM, Richard Fuchs wrote:
-int db_mysql_free_result(db1_con_t* _h, db1_res_t* _r); +int db_mysql_free_result(const db1_con_t* _h, db1_res_t* _r);
the change to the prototype of the above function is now throwing compilation warning:
km_db_mysql.c: In function ‘db_mysql_bind_api’: km_db_mysql.c:109: warning: assignment from incompatible pointer type
Do you need the const specifier? If yes, the DB API and probably the other database drivers need to be updated.
The const is needed because db_mysql_free_result is called from other functions which have the db1_con_t object declared as const. Removing the const from the prototype would trigger warnings elsewhere.
The idea is that the driver-specific result set should really be stored in the db1_res_t object, and not in db1_con_t as the other drivers do it now (which was causing the segfault). Therefore, ->free_result really has no reason to modify the db1_con_t object, which warrants the const modifier. Preferably the other drivers should be updated to follow the same reasoning, after which the const can be put into db_func_t, but I don't have enough experience with them to do it myself.
Alternatively, the const could probably be removed from this and a few other function prototypes, or maybe a wrapper function could be used.
cheers
Hello,
On 4/29/13 3:01 PM, Richard Fuchs wrote:
Hi,
On 04/29/13 07:11, Daniel-Constantin Mierla wrote:
Hello,
On 4/10/13 3:36 PM, Richard Fuchs wrote:
-int db_mysql_free_result(db1_con_t* _h, db1_res_t* _r); +int db_mysql_free_result(const db1_con_t* _h, db1_res_t* _r);
the change to the prototype of the above function is now throwing compilation warning:
km_db_mysql.c: In function ‘db_mysql_bind_api’: km_db_mysql.c:109: warning: assignment from incompatible pointer type
Do you need the const specifier? If yes, the DB API and probably the other database drivers need to be updated.
The const is needed because db_mysql_free_result is called from other functions which have the db1_con_t object declared as const. Removing the const from the prototype would trigger warnings elsewhere.
The idea is that the driver-specific result set should really be stored in the db1_res_t object, and not in db1_con_t as the other drivers do it now (which was causing the segfault). Therefore, ->free_result really has no reason to modify the db1_con_t object, which warrants the const modifier. Preferably the other drivers should be updated to follow the same reasoning, after which the const can be put into db_func_t, but I don't have enough experience with them to do it myself.
Alternatively, the const could probably be removed from this and a few other function prototypes, or maybe a wrapper function could be used.
your patch is incomplete/wrong in a way or another -- you introduced a warning in the piece of code you changed. It will be still wrong if the warning will be in another place, so choosing one instead of the other is not a solution.
You can probably cast to const whenever it is required so by a calling function, rather than changing the prototype and leaving it with warning like this, if you don't know how to/can't update the other drivers for various reasons.
Cheers, Daniel
-
Daniel-Constantin Mierla -
Juha Heinanen -
Richard Fuchs