List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:February 4 2011 10:36am
Subject:Re: bzr commit into mysql-5.5 branch (alexander.barkov:3294)
View as plain text  
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.

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.

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