List:Commits« Previous MessageNext Message »
From:Konstantin Osipov Date:April 20 2010 7:32pm
Subject:Re: bzr commit into mysql-5.0-bugteam branch (davi:2858) Bug#50974
View as plain text  
* Davi Arnaut <Davi.Arnaut@stripped> [10/04/20 04:46]:
>  2858 Davi Arnaut	2010-04-19
>       Bug#50974: Server keeps receiving big (> max_allowed_packet) packets
> indefinitely.
>       
>       The server could be tricked to read packets indefinitely if it
>       received a packet larger than the maximum size of one packet.
>       This problem is aggravated by the fact that it can be triggered
>       before authentication.

>       The solution is to skip at least twice the maximum packet size.

>  
> +
> +/*
> +  Helper function to determine the skip factor.
> +*/
> +
> +static ulonglong my_net_skip_rest_factor(NET *net)
> +{
> +#if defined(MYSQL_SERVER) && !defined(EMBEDDED_LIBRARY)
> +  return net->max_packet_size * net->skip_rest_factor;
> +#else
> +  return net->max_packet_size * 2;
> +#endif
> +}

Should be #ifdef MYSQL_SERVER perhaps, since it is only
used for MYSQL_SERVER && ! EMBEDDED_LIBRARY.

> @@ -744,6 +762,7 @@ static my_bool my_net_skip_rest(NET *net
>  				ALARM *alarm_buff)
>  {
>    uint32 old=remain;
> +  ulonglong skippable= my_net_skip_rest_factor(net);
>    DBUG_ENTER("my_net_skip_rest");
>    DBUG_PRINT("enter",("bytes_to_skip: %u", (uint) remain));
>  
> @@ -759,6 +778,12 @@ static my_bool my_net_skip_rest(NET *net
>    }
>    for (;;)
>    {
> +    /* Don't read packets indefinitely. */
> +    if (remain >= skippable)
> +      DBUG_RETURN(1);
> +    else
> +      skippable-= remain;

I think we should not limit how much we skip
the trail for an authenticated user. After all, 
he/she could be trying to send a large blob.
Instead, I would limit how many times we allow
this type of error to occur: set it to 5, 10,
or actually infinity, i.e. do not fix the bug
at all for the case when the user is authenticated.


> +
>      while (remain > 0)
>      {
>        uint length= min(remain, net->max_packet);
> @@ -769,9 +794,12 @@ static my_bool my_net_skip_rest(NET *net
>      }
>      if (old != MAX_PACKET_LENGTH)
>        break;
> +    else if (skippable < NET_HEADER_SIZE)
> +      DBUG_RETURN(1);
>      if (net_safe_read(net, (char*) net->buff, NET_HEADER_SIZE, alarmed))
>        DBUG_RETURN(1);
>      old=remain= uint3korr(net->buff);
> +    skippable-= NET_HEADER_SIZE;

OK, this code is correct and always checking skippable before 
subtraction. Perhaps a matter of taste, but I would (in addition)
use a signed type as in the original patch. It's networking, so we
could be a little more on the side of defensive programming.

>      net->pkt_nr++;
>    }
>    DBUG_RETURN(0);
> 
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc	2010-02-25 14:57:15 +0000
> +++ b/sql/sql_parse.cc	2010-04-20 00:42:25 +0000
> @@ -493,6 +493,8 @@ int check_user(THD *thd, enum enum_serve
>        }
>        send_ok(thd);
>        thd->password= test(passwd_len);          // remember for error messages 
> +      /* Skip at most 2 * max_packet_size bytes. */
> +      thd->net.skip_rest_factor= 2;

Please if you decide to use this variable explain
exactly what is happening here. Let's discuss on
IRC if you need help with the comment.

Please also add the description of the life cycle
and all use cases utilizing the variable in its declaration.

-- 
Thread
bzr commit into mysql-5.0-bugteam branch (davi:2858) Bug#50974Davi Arnaut20 Apr
  • Re: bzr commit into mysql-5.0-bugteam branch (davi:2858) Bug#50974Konstantin Osipov20 Apr
Re: bzr commit into mysql-5.0-bugteam branch (davi:2858) Bug#50974Konstantin Osipov29 Apr