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
