#306 db_mysql multiple issues in run_mysql_cmd wrapper

modules (454)

There seem to be a couple of issues hidden in `run_mysql_cmd` macro:

- if there is no connection, first `if` causes a fall through to checking `mysql_errno()`, which has the return value of the last command, while it assumes checking the (_cmd)'s return code

- the macro is used for both prepared statement and other commands, even though mysql_stmt_* commands report the code via mysql_stmt_errno(), not mysql_errno()

- comparing mysql_errno to error codes and then getting mysql_error when error code==0 seems like a stupid thing to do, unless it's a workaround for some libmysqlclient bug

- "disconect" should be spelled "disconnect" :)

- reported errors are not true in functions using the wrapper - for example re_init_statement reports `if (code<0) { LM_ERR("failed while mysql_stmt_prepare()\n"); ...` while it can fail on db_mysql_connect which also returns the result via the 'code' variable

- if the database disconnectts during mysql_stmt_reppare, statements are going to be reset (reset_all_statements()), the final value of code will be that of db_mysql_connect (probably 0) and ctx->stmt will not be initialised, but mysql_stmt_close() will be run on it, causing a crash

- probably some others I haven't noticed...

Proposed solution - kill the macro and rewrite it in a sane way - I tried fixing the separate problems, but it seems like there are too many issues with it.


  • Stanislaw Pitucha

    Attached a proposed way to change the code. It's only a couple of lines longer in place of the macro, but the logic is more explicit and the function-specific wrappers will remove problems related to error checking the return codes. Tested on a specifically prepared mysql which drops the connections tcp quite often - doesn't seem to cause crashes anymore.

  • Stanislaw Pitucha

    Uploaded my patch queue. It removes all occurrences of run_mysql_cmd and replaces them with some common code. (they can be macro'd if someone really wants to do that)
    From the limited testing I could do, it seem the patches work just fine. To simulate the disconnect conditions, you can setup haproxy with very low idle client disconnection time in front of the database itself.

  • Bogdan-Andrei Iancu

    Hi Stan,

    Which patch is the final one? or all of them ?

    Just to know where to start :)


  • Bogdan-Andrei Iancu

    • priority: 5 --> 9
    • assigned_to: nobody --> bogdan_iancu
  • Stanislaw Pitucha

    All of them, to be applied in order. They're just split into the same chunks, how I created / tested them. If it doesn't improve your review, just apply all of them :)

  • Bogdan-Andrei Iancu

    Hi Stan,

    Reviewed the fixes and look good - I uploaded your patches on trunk and 1.6

    Many thanks & Best regards,

  • Bogdan-Andrei Iancu

    • status: open --> closed-fixed

Log in to post a comment.