From: Sergey Vojtovich Date: February 4 2011 2:50pm Subject: Re: bzr commit into mysql-5.5 branch (alexander.barkov:3294) List-Archive: http://lists.mysql.com/commits/130425 Message-Id: <20110204145018.GA18888@june> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Hi Alexander, comments inline. :) On Fri, Feb 04, 2011 at 01:57:49PM +0300, Alexander Barkov wrote: > Hi Joro, Sergey, > > Can you please review my patch for Bug#58036? > > The full patch for 5.5 is here: > > http://lists.mysql.com/commits/130143 > > (I'll sent a patch for 5.1 separately, as the > affected code was significantly different in 5.1) > > > Thanks! > > > Sergey, thanks for review! > > Please see my comments inline: > > > Sergey Vojtovich wrote: > >Alexander, > > > >ok, now I understand what is it all about. But I must admit that it is > >something contrary to evidence. > > > >I approve the patch with two remarks: > > > >1. In sql_parse.cc there is "my_free(thd->security_ctx->user);" > > But user may be NULL and AFAIR free() doesn't always like NULL. > > free() likes NULL in C89/C99. Ok. After all I can't recall why it could be a problem. Though looking at 5.1 I can see MY_ALLOW_ZERO_PTR. Probably something safemalloc related. > > > > > >2. I think there is another, cleaner way to solve this double free > > problem: set thd->security_ctx->user to NULL in sql_parse.cc. But > > I think it is a good idea to ask Joro what is the right way of > > doing things and to do second review. > > Possibly. My reasoning was, as far as acl_authentificate() is called in > at least two places, it's fair to say that both places > expect thd->security_ctx->user to be alloced (or to be NULL), > and they both expect than thd->security_ctx->user never stays untouched. > So this is a minimal fix to cover both calls. Well, we're actually talking about parse_com_change_user_packet(), which is called only when command is COM_CHANGE_USER. The second place, which is COM_CONNECT must not expect that. Regards, Sergey > If Joro suggests a better fix for this problem, > then I'll be happy to incorporate into my patch: > > > + > + /* > + The caller expects that this function allocates user_name using > + my_strndup() and calls my_free(user_name) later in some cases. > + > + Let's set user_name to NULL here, to avoid my_free() on uninitialized > + memory for those cases when we return from here *before* user_name is > + actually allocated. This happens on errors: packet error, bad charset. > + */ > + mpvio->auth_info.user_name= NULL; > + mpvio->auth_info.user_name_length= 0; > + > > Thanks! > > > > > >Regards, > >Sergey > > > >On Fri, Feb 04, 2011 at 10:59:07AM +0300, Alexander Barkov wrote: > >>Hi Sergey, > >> > >>Sergey Vojtovich wrote: > >>>Hi Alexander, > >>> > >>>still, there is one question... > >>> > >>>On Tue, Feb 01, 2011 at 03:36:16PM -0000, Alexander Barkov wrote: > >>>>#At file:///home/bar/mysql-bzr/mysql-5.5.b58036/ based on revid:ole.john.aske@stripped > >>>> > >>>>3294 Alexander Barkov 2011-02-01 > >>>BUG#.... line is missing. > >>> > >>>> Problem: ucs2 was correctly disallowed in "SET NAMES" only, > >>>> while mysql_real_connect() and mysql_change_user() still allowed > >>>> to use ucs2, which made server crash. > >>>...skip... > >>> > >>>>=== modified file 'sql/sql_acl.cc' > >>>>--- a/sql/sql_acl.cc 2011-01-14 15:48:11 +0000 > >>>>+++ b/sql/sql_acl.cc 2011-02-01 15:30:06 +0000 > >>>>@@ -7799,7 +7799,8 @@ public: > >>>> Thd_charset_adapter(THD *thd_arg) : thd (thd_arg) {} bool > >>>>init_client_charset(uint cs_number) > >>>> { > >>>>- thd_init_client_charset(thd, cs_number); > >>>>+ if (thd_init_client_charset(thd, cs_number)) > >>>>+ return true; > >>>> thd->update_charset(); > >>>> return thd->is_error(); > >>>> } > >>>>@@ -8236,6 +8237,18 @@ static bool parse_com_change_user_packet > >>>> uint dummy_errors; > >>>> DBUG_ENTER ("parse_com_change_user_packet"); > >>>>+ > >>>>+ /* > >>>>+ The caller expects that this function allocates user_name using > >>>>+ my_strndup() and calls my_free(user_name) later in some cases. > >>>>+ > >>>>+ Let's set user_name to NULL here, to avoid my_free() on uninitialized > >>>>+ memory for those cases when we return from here *before* user_name is > >>>>+ actually allocated. This happens on errors: packet error, bad charset. > >>>>+ */ > >>>>+ mpvio->auth_info.user_name= NULL; > >>>>+ mpvio->auth_info.user_name_length= 0; > >>>>+ > >>>> if (passwd >= end) > >>>> { > >>>> my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0)); > >>>> > >>>When does the caller set auth_info.user_name to not NULL and when does the > >>>caller free it? > >> > >>Normally, parse_com_change_user_packet() does the following: > >> > >> /* we should not free mpvio->user here: it's saved by > >>dispatch_command() */ > >> if (!(mpvio->auth_info.user_name= my_strndup(user_buff, user_len, > >> MYF(MY_WME) > >> return 1; > >> > >> > >>So it either allocates user_name or sets it to NULL > >>on my_strndup failure. > >> > >> > >>Now suppose we exited earlier, before my_strndup() happened. > >> > >> > >> > >> > >>bool dispatch_command(...) > >>{ > >>... > >> > >> case COM_CHANGE_USER: > >> > >> .... > >> Security_context save_security_ctx= *thd->security_ctx; > >> .... > >> rc= acl_authenticate(thd, 0, packet_length); > >> ... > >> if (rc) > >> { > >> ... > >> /* > >> If acl_authenticate() returned before my_strndup(), then > >> thd->security_ctx->user points to the original user value > >> which was when we entered this function. > >> But! save_security_ctx->user also points to the same user string. > >> */ > >> > >> my_free(thd->security_ctx->user); // We assume it was alloced here! > >> *thd->security_ctx= save_security_ctx; > >> > >> /* > >> Now thd->security_ctx points to already freed memory > >> */ > >> ... > >> } > >> > >> > >> > >> > >>Then, sooner or later, THD executes: > >> > >> thd->security_ctx::destroy(), > >> > >>which does my_free(user) on the previously freed address. > >> > >> > > > -- Sergey Vojtovich MySQL AB, Software Engineer Izhevsk, Russia, www.mysql.com