10 Apr
2013
10 Apr
'13
3:36 p.m.
Module: sip-router
Branch: master
Commit: 94ce2b1de63432baaaecd9285608380a40a70550
URL:
http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=94ce2b1…
Author: Richard Fuchs <rfuchs(a)sipwise.com>
Committer: Richard Fuchs <rfuchs(a)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);
10 Apr
10 Apr
5:12 p.m.
New subject: git:master: db_mysql: fix segfault when recursive queries are made
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
5:31 p.m.
New subject: git:master: db_mysql: fix segfault when recursive queries are made
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
6:02 p.m.
New subject: git:master: db_mysql: fix segfault when recursive queries are made
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
6:08 p.m.
New subject: git:master: db_mysql: fix segfault when recursive queries are made
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
6:17 p.m.
New subject: git:master: db_mysql: fix segfault when recursive queries are made
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
29 Apr
29 Apr
1:11 p.m.
New subject: git:master: db_mysql: fix segfault when recursive queries are made
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
--
Daniel-Constantin Mierla - http://www.asipto.com
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Kamailio Advanced Training, San Francisco, USA - June 24016, 2013
* http://asipto.com/u/katu *
3:01 p.m.
New subject: git:master: db_mysql: fix segfault when recursive queries are made
Hi,
On 04/29/13 07:11, Daniel-Constantin Mierla wrote:
Hello,
On 4/10/13 3:36 PM, Richard Fuchs wrote:
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
-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.
3:31 p.m.
New subject: git:master: db_mysql: fix segfault when recursive queries are made
Hello,
On 4/29/13 3:01 PM, Richard Fuchs wrote:
Hi,
On 04/29/13 07:11, Daniel-Constantin Mierla wrote:
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 - http://www.asipto.com
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Kamailio Advanced Training, San Francisco, USA - June 24016, 2013
* http://asipto.com/u/katu *
Hello,
On 4/10/13 3:36 PM, Richard Fuchs wrote:
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. -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.
4022
days inactive
4041
days old
8 comments
3 participants
participants (3)
-
Daniel-Constantin Mierla -
Juha Heinanen -
Richard Fuchs