[Kamailio-Devel] MySQL (DB) issue

[Jeremy Snapcase]

Hi,

We're using a setup with OpenSER 1.3.4 and multiple MySQL databases.
This particular setup gives rise to issues with the MySQL database
module. The issue possibly extends to other databases aswell.

Before investigating, I first thought the issue lay elsewhere. The
setup worked perfectly fine until I started reading AVP's from
database.. My first thought was the avp_db module was at fault. After
more testing, it turned out the auth_db module was crashing OpenSER.

==29228==    at 0x8FE1910: get_ha1 (authorize.c:115)
==29228==    by 0x8FE111D: authorize (authorize.c:236)
==29228==    by 0x8FE1A60: www_authorize (authorize.c:279)
==29228==    by 0x40FB0C: do_action (action.c:821)
==29228==    by 0x40CEA9: run_action_list (action.c:132)
==29228==    by 0x44B010: eval_elem (route.c:1096)
==29228==    by 0x44EE90: eval_expr (route.c:1387)
==29228==    by 0x44EF74: eval_expr (route.c:1403)
==29228==    by 0x44EFA8: eval_expr (route.c:1408)
==29228==    by 0x40F379: do_action (action.c:677)
==29228==    by 0x40CEA9: run_action_list (action.c:132)
==29228==    by 0x40F4EC: do_action (action.c:700)
==29228==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

I looked at the source code for auth_db but couldn't find anything
suspicious. Especially not at the location valgrind gave me.

Then I started suspecting the database code, because at exit,
sometimes other modules would start to receive weird results. Instead
of phone numbers, usernames were returned, etc.. So I took a look at
OpenSER's database code, and noticed something odd.

I'll describe what I saw while looking at the MySQL code, but I guess
this applies to other modules aswell.

db_mysql_store_result(db_con_t* _h, db_res_t** _r)

	[ Called for each select query. Converts MySQL resource to db_res_t,
and frees result ]

	-> db_mysql_convert_result

	  229 	while( mysql_next_result( CON_CONNECTION(_h) ) > 0 ) {
	  230 		MYSQL_RES *res = mysql_store_result( CON_CONNECTION(_h) );
	  231 		mysql_free_result( res );
	  232 	}

db_mysql_convert_result(db_con_t* _h, db_res_t* _r)

	[ Converts MySQL resource to db_res_t ]

	-> db_mysql_convert_rows

db_mysql_convert_rows(db_con_t* _h, db_res_t* _r)

	[ Converts each row ]

	-> db_mysql_convert_row

db_mysql_convert_row(db_con_t* _h, db_res_t* _res, db_row_t* _r)

	[ Converts a single row. Passes MySQL row value to str2val ]

	-> str2val(
		RES_TYPES(_res)[i],
		&(ROW_VALUES(_r)[i]),
		((MYSQL_ROW)CON_ROW(_h))[i], <-- argument passed directly from MySQL resource.
		lengths[i]) < 0)

str2val(db_type_t _t, db_val_t* _v, const char* _s, int _l)

	[ Converts MySQL row value to db_val_t ]

	NOTE: _s is a pointer to a MYSQL_ROW.. The ownership of the string
lies with the MySQL client.
	NOTE: _v, a member of db_res_t, is assigned a pointer the _s. If the
type is DB_STRING or DB_STR.

	   91 	case DB_STRING:
	   92 		VAL_STRING(_v) = _s;
	   93 		VAL_TYPE(_v) = DB_STRING;
	   94 		return 0;
	   95
	   96 	case DB_STR:
	   97 		VAL_STR(_v).s = (char*)_s;
	   98 		VAL_STR(_v).len = _l;
	   99 		VAL_TYPE(_v) = DB_STR;
	  100 		return 0;

Note that the MySQL module first converts the results to a db_res_t,
assigning directly references to data held by MySQL, and then goes on
the free those very same results (!). Of course, this won't crash
OpenSER directly.. The MySQL client won't actually free the result. It
will recycle it instead, meaning the data can still be read after the
result has been freed.

It does leave a window though, where MySQL decides to recycle the
resource, while the result hasn't been yet consumed.

Eg,

	authenticate:

		get stored credentials
			(mysql query)
			(mysql convert)
			(mysql free)

		[ window ]

		check credentials
			read db_res_t
			free db_res_t

If the resource has been recycled between (free) and being consumed,
the credentials check may use improper data, or even crash OpenSER,
which is exactly what happened in our setup. This window is very
small, much smaller than it takes for MySQL to complete another
query.. Also, it only happens with strings, as primitive types are
copied, rather than referenced. This explains why this issue is so
hard to notice. It began happening when we intensified the number of
database queries involving strings, while at the same time, slowing
down OpenSER (it runs on a VM).

The solution is to either:
-	Copy the data, no longer requiring the MySQL resource and having a
copy of our own.
-	Or, tie the lifetime of the MySQL resource to the lifetime of db_res_t.

The second solution is very simple to achieve. Simply add an
'on_destroy' callback to db_res_t, which gets called when db_res_t is
freed. Upon conversion, the database module sets this callback, which
enables the database module to be notified when the resource is
destroyed, and free the MySQL resource. This guarantees the lifetime
of the MySQL resource >= db_res_t.

	(mysql query)
	(mysql convert)
		db_res_t->on_destroy = mysql_db_free_result
		db_res_t->client_data = mysql_res

	read db_res_t [ consume ]

	free db_res_t
		db_res_t->on_destroy(db_res_t->client_data)
			(mysql_db_free_result)
				(mysql free)

The first solution is troublesome.. db_val_t is not the owner of the
strings it holds.. We have no way of knowing whether it's safe to free
the string or not.