MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Jim Winstead Date:January 25 2008 12:53am
Subject:Re: bk commit into 4.1 tree (cmiller:1.2706) BUG#33841
View as plain text  
On Thu, Jan 24, 2008 at 07:04:53PM -0500, Chad MILLER wrote:
> ChangeSet@stripped, 2008-01-24 19:04:52-05:00, cmiller@stripped +1 -0
>   Bug#33841: mysql client crashes when returning results for long-\
>   	running queries
>   
>   Bug#33976: buffer overflow of variable time_buff in function com_go()
>   
>   Two separate problems:  One internal buffer was too short by design,
>   and the other could be appended to arbitrarily many times.  In
>   both cases, that could smash the stack on some architectures and
>   cause SEGVs.  This is not a problem that could be exploited to 
>   run arbitrary code.

i don't see how this is true. on each iteration through the loop, buff
is written to at its head, and then more data appended. but there are
only limited bits of data that may be written.

>   To fix, I expanded one buffer to cover all the size that could be
>   written to (we know the abolute max).  In the other case, we now 
>   grow the buffer as we need it to contain the message(s).

i think both buffers could be fixed-size.

> diff -Nrup a/client/mysql.cc b/client/mysql.cc
> --- a/client/mysql.cc	2006-11-02 07:24:58 -05:00
> +++ b/client/mysql.cc	2008-01-24 19:04:50 -05:00
> @@ -1,4 +1,4 @@
> -/* Copyright (C) 2000-2003 MySQL AB
> +/* Copyright (C) 2000-2008 MySQL AB
>  
>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
> @@ -729,7 +729,7 @@ static void usage(int version)
>    if (version)
>      return;
>    printf("\
> -Copyright (C) 2002 MySQL AB\n\
> +Copyright (C) 2008 MySQL AB\n\

may as well make this 2000-2008, too.

>  This software comes with ABSOLUTELY NO WARRANTY. This is free software,\n\
>  and you are welcome to modify and redistribute it under the GPL license\n");
>    printf("Usage: %s [OPTIONS] [database]\n", my_progname);
> @@ -1910,7 +1910,8 @@ com_charset(String *buffer __attribute__
>  static int
>  com_go(String *buffer,char *line __attribute__((unused)))
>  {
> -  char		buff[200], time_buff[32], *pos;
> +  char		*buff, time_buff[49+3+1];

should have comment explaining 49+3+1.

> +  ulong	buff_allocated_size;
>    MYSQL_RES	*result;
>    ulong		timer, warnings;
>    uint		error= 0;
> @@ -1923,7 +1924,6 @@ com_go(String *buffer,char *line __attri
>    }
>  
>    /* Remove garbage for nicer messages */
> -  LINT_INIT(buff[0]);
>    remove_cntrl(*buffer);
>  
>    if (buffer->is_empty())
> @@ -1971,24 +1971,34 @@ com_go(String *buffer,char *line __attri
>    error=0;
>    buffer->length(0);
>  
> +  buff_allocated_size= 400;  /* largest until realloc is about 35 */
> +  buff= my_malloc(buff_allocated_size, MYF(MY_WME));
> +  LINT_INIT(buff[0]);
> +
>    do
>    {
> +    ulong buff_used;
> +    char *pos;
> +
>      if (quick)
>      {
>        if (!(result=mysql_use_result(&mysql)) &&
> mysql_field_count(&mysql))
> -	return put_error(&mysql);
> +      {
> +	error= put_error(&mysql);
> +	goto free_and_return;
> +      }
>      }
>      else
>      {
>        error= mysql_store_result_for_lazy(&result);
>        if (error)
> -	return error;
> +	goto free_and_return;
>      }
>  
>      if (verbose >= 3 || !opt_silent)
>        mysql_end_timer(timer,time_buff);
>      else
> -      time_buff[0]=0;
> +      time_buff[0]= '\0';
>      if (result)
>      {
>        if (!mysql_num_rows(result) && ! quick)
> @@ -2023,7 +2033,17 @@ com_go(String *buffer,char *line __attri
>  	      (long) mysql_affected_rows(&mysql),
>  	      (long) mysql_affected_rows(&mysql) == 1 ? "row" : "rows");
>  
> -    pos=strend(buff);
> +    buff_used = strlen(buff);
> +
> +    /* len("Query OK, 4294967296 rows affected, 4294967296 warnings, 4294967296
> days, 23 hours, 59 minutes, 60 seconds")+1 == 107 */
> +#define MAX_STATUS_LENGTH 107

so why aren't we okay with buff[200]? or maybe it is only time_buff that
is getting smashed, never buff.

the most that has been written to buff at this point is "Query OK,
4294967296 rows affected"

jim

> +    if ((buff_used + MAX_STATUS_LENGTH) > buff_allocated_size)
> +    {
> +      buff_allocated_size += MAX_STATUS_LENGTH;
> +      buff= my_realloc(buff, buff_allocated_size, MYF(MY_FAE+MY_WME));
> +    }
> +    pos= buff + buff_used;
> +
>      if ((warnings= mysql_warning_count(&mysql)))
>      {
>        *pos++= ',';
> @@ -2045,6 +2065,7 @@ com_go(String *buffer,char *line __attri
>        fflush(stdout);
>      mysql_free_result(result);
>    } while (!(err= mysql_next_result(&mysql)));
> +
>    if (err >= 1)
>      error= put_error(&mysql);
>  
> @@ -2052,6 +2073,9 @@ com_go(String *buffer,char *line __attri
>        (mysql.server_status & SERVER_STATUS_DB_DROPPED))
>      get_current_db();
>  
> +free_and_return:
> +  my_free(buff, MYF(0));
> +
>    return error;				/* New command follows */
>  }
>  
> @@ -3275,6 +3299,11 @@ static ulong start_timer(void)
>  }
>  
>  
> +/** 
> +  Write as many as 49+1 bytes to buff, in the form of a legible duration of time.
> +
> +  len("1193046 days, 23 hours, 59 minutes, 60.00 seconds")  ->  49
> +*/
>  static void nice_time(double sec,char *buff,bool part_second)
>  {
>    ulong tmp;
Thread
bk commit into 4.1 tree (cmiller:1.2706) BUG#33841Chad MILLER25 Jan
  • Re: bk commit into 4.1 tree (cmiller:1.2706) BUG#33841Jim Winstead25 Jan