Hi Mats,
thanks for the review, answers inline.
On Fri, Aug 20, 2010 at 03:07:50PM +0200, Mats Kindahl wrote:
> Hi Svoj,
>
> Sorry it took some time, I had to go through the audit code to see how
> it worked.
>
> Patch approved.
>
> I have some minor comments on the comments. :)
>
> I leave it up to you to decide if you want to change them.
>
> /Matz
>
> On 08/20/2010 11:58 AM, Sergey Vojtovich wrote:
> > #At file:///home/svoj/mysql/server/mysql-5.5-bugfixing-bug54989/ based on
> revid:alik@stripped
> >
> > 3186 Sergey Vojtovich 2010-08-20
> > BUG#54989 - With null_audit installed, server hangs on an
> > attempt to install a plugin twice
> >
> > Server crashes when [UN]INSTALL PLUGIN fails (returns an
> > error) and general log is disabled and there are audit
> > plugins interested in MYSQL_AUDIT_GENERAL_CLASS.
> >
> > When audit event is triggered, audit subsystem acquires interested
> > plugins by walking through plugin list. Evidently plugin list
> > iterator protects plugin list by acquiring LOCK_plugin, see
> > plugin_foreach_with_mask().
> >
> > On the other hand [UN]INSTALL PLUGIN is acquiring LOCK_plugin
> > rather for a long time.
> >
> > When audit event is triggered during [UN]INSTALL PLUGIN, plugin
> > list iterator acquires the same lock (within the same thread)
> > second time.
> >
> > Repeatable only with general_log disabled, because general_log
> > triggers MYSQL_AUDIT_GENERAL_LOG event, which acquires audit
> > plugins before [UN]INSTALL PLUGIN acquired LOCK_plugin.
> >
>
> After?
No.
> From the stack trace in the bug report, it seems like the audit
> subsystem tries to acquire the lock (inside mysql_audit_notify) *after*
> the INSTALL PLUGIN has acquired LOCK_plugin (which is done inside
> mysql_install_plugin before plugin_add is called, which later calls
> mysql_audit_notify).
Absolutely correct for MYSQL_AUDIT_GENERAL_ERROR event, without preceding
MYSQL_AUDIT_GENERAL_LOG event. Here I'm talking why we're sensitive to
general_log state and about MYSQL_AUDIT_GENERAL_LOG, which is triggered
*before* LOCK_plugin is acquired.
...skip...
> > + This hack should be removed when LOCK_plugin is fixed so it
> > + protects only what it supposed to protect.
> >
>
> It would be nice if you could elaborate on what LOCK_plugin should
> protect. It will make it easier to fix the issue in the future.
The answer is simple: I don't know. The code is evolving in rather
chaotic way. I guess these days there is no person actively working
with this code, nor a set of rules regulating it.
Though there is a comment in sql_plugin.cc, which says:
/*
A mutex LOCK_plugin must be acquired before accessing the
following variables/structures.
We are always manipulating ref count, so a rwlock here is unneccessary.
*/
mysql_mutex_t LOCK_plugin;
static DYNAMIC_ARRAY plugin_dl_array;
static DYNAMIC_ARRAY plugin_array;
static HASH plugin_hash[MYSQL_MAX_PLUGIN_TYPE_NUM];
static bool reap_needed= false;
static int plugin_array_version=0;
...skip...
Regards,
Sergey
--
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
