List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:August 20 2010 3:20pm
Subject:Re: bzr commit into mysql-5.5-bugfixing branch (svoj:3186) Bug#54989
View as plain text  
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
Thread
bzr commit into mysql-5.5-bugfixing branch (svoj:3186) Bug#54989Sergey Vojtovich20 Aug
  • Re: bzr commit into mysql-5.5-bugfixing branch (svoj:3186) Bug#54989Mats Kindahl20 Aug
    • Re: bzr commit into mysql-5.5-bugfixing branch (svoj:3186) Bug#54989Sergey Vojtovich20 Aug