[Kamailio-Devel] MySQL (DB) issue

[Henning Westerholt]

On Wednesday 10 December 2008, Jeremy Snapcase wrote:
> 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.

Hi Jeremy,

thanks for the detailed report.

> 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==  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.

You're using OpenSER without the PKG_MALLOC memory manager? Because otherwise 
i'd think that valgrind would not work correctly.

> 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.

It applies to the mysql and unixodbc modules, the postgres module for example 
uses a different approach.

> 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 );
> [..]
> 		((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.
> [..]

Correct.

> 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.
> [..]
> 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).

Correct. I think this happens really rarely, and perhaps only under the 
special circumstance you created, as you're the first person that reports a 
crash caused by this. We're using it in production with a lot of load, and 
have not seen this yet. Perhaps its also something related to the version of 
the client library. I'd guess this behaviour was choosen years ago because of 
performance reasons, to avoid the copy that seemed unnecessary.

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

This is the approach choosen from the postgres module and some other non-SQL 
modules.

> -	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.
> [..]
> 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.

The DB API in 1.4.x is different, it allows to decide from the db_val_t if 
this string needs to be freed or not. Because of this i'd favor the first 
approach, as this would align the behaviour of the mysql module to the other 
DB modules. I'll investigate this.

How often do you observe crashes because of this problem? Would it be a 
problem for you to update to a 1.4 based release? The 1.3 branch will be not 
that long supported anymore, as the release of 1.5.0 is planned in the 
beginning of the next year. As this problem was not noticed that long, i 
don't want to introduce too much instability in this branch, if i'm 
honest. ;-)

Cheers,

Henning