List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:February 4 2011 2:50pm
Subject:Re: bzr commit into mysql-5.5 branch (alexander.barkov:3294)
View as plain text  
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 <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
Thread
bzr commit into mysql-5.5 branch (alexander.barkov:3294) Alexander Barkov1 Feb
  • Re: bzr commit into mysql-5.5 branch (alexander.barkov:3294)Sergey Vojtovich4 Feb
    • Re: bzr commit into mysql-5.5 branch (alexander.barkov:3294)Alexander Barkov4 Feb
      • Re: bzr commit into mysql-5.5 branch (alexander.barkov:3294)Alexander Barkov4 Feb
      • Re: bzr commit into mysql-5.5 branch (alexander.barkov:3294)Sergey Vojtovich4 Feb
        • Re: bzr commit into mysql-5.5 branch (alexander.barkov:3294)Alexander Barkov4 Feb
          • Re: bzr commit into mysql-5.5 branch (alexander.barkov:3294)Sergey Vojtovich4 Feb
Re: bzr commit into mysql-5.5 branch (alexander.barkov:3294)Alexander Barkov10 Feb