Bug #48983 Bad strmake calls (length one too long)
Submitted: 23 Nov 2009 10:41 Modified: 12 Mar 2010 16:47
Reporter: Mattias Jonsson Email Updates:
Status: Closed Impact on me:
None 
Category:MySQL Server: General Severity:S2 (Serious)
Version:5.0, 5.1, 5.6 bzr OS:Any
Assigned to: Davi Arnaut CPU Architecture:Any
Triage: Triaged: D2 (Serious)

[23 Nov 2009 10:41] Mattias Jonsson
Description:
Found this when reporting bug#48939. New version of bug#31752.

strmake appends '\0' at the end of the destination string. And the resulting string is allowed to be 'length' long, resulting in when source string length is 'length' or longer, the buffer for destination string must be 'length' + 1 characters long.

I found several suspicious calls to strmake where the length is one byte too long (should be DEFINED_LENGHT - 1, instead of DEFINED_LENGTH). These occurrences of bad calls may lead corrupt data, crashes etc (since it allows for writing '\0' i the memory area starting after the destination buffer).

How to repeat:
Read the source...

Suggested fix:
Go through all strmake calls and verify the length argument.
[23 Nov 2009 10:43] Mattias Jonsson
Here is some notes from my own findings of bad strmake calls:
libmysql/libmysql.c mysql_change_user uses a too small buffer

char buff[USERNAME_LENGTH+SCRAMBLED_PASSWORD_CHAR_LENGTH+NAME_LEN+2];
char *end= buff;
end= strmake(end, user, USERNAME_LENGTH) + 1;
/* now USERNAME_LENGTH + 1 */
  if (passwd[0])
  {
    if (mysql->server_capabilities & CLIENT_SECURE_CONNECTION)
    {
      *end++= SCRAMBLE_LENGTH;
      scramble(end, mysql->scramble, passwd);
      end+= SCRAMBLE_LENGTH;
/* now USERNAME_LENGTH + 1 + 1 + SCRAMBLE_LENGTH (20) =>
   USERNAME_LENGTH + 1 + 1 + SHA1_HASH_SIZE (20)
   OK, always at least 19 bytes too long instead of too short...
*/
end= strmake(end, db ? db : "", NAME_LEN) + 1;
if (mysql->server_capabilities & CLIENT_SECURE_CONNECTION)
{
  int2store(end, (ushort) mysql->charset->number);
  end+= 2;
}

max is now USERNAME_LENGTH + 1 + 1 + SCRAMBLE_LENGTH + NAME_LEN + 1 + 2      

read_user_name:
strmake(name, str ? str : "UNKNOWN_USER", USERNAME_LENGTH);
  
mysql_real_connect uses too small buffer:
char name_buff[USERNAME_LENGTH];
...
read_user_name(name_buff);
Also in sql-common/client.c (but may be OK?!?!)
  if (user && user[0])
    strmake(end,user,USERNAME_LENGTH);          /* Max user name */
  else
    read_user_name((char*) end);
Maybe OK, since it uses 'end' after this too...

mysql_list_fields(MYSQL *mysql, const char *table, const char *wild)
{
  MYSQL_RES   *result;
  MYSQL_FIELD *fields;
  char       buff[257],*end;
  DBUG_ENTER("mysql_list_fields");
  DBUG_PRINT("enter",("table: '%s'  wild: '%s'",table,wild ? wild : ""));

  end=strmake(strmake(buff, table,128)+1,wild ? wild : "",128);

If table and wild is >= 128 then buf[258] (*end) ? will be overwritten!

in server-tools/instance-manager/instance_map.cc:
static void parse_option(const char *option_str,
                         char *option_name_buf,
                         char *option_value_buf)
{
  const char *eq_pos;
  const char *ptr= option_str;

  while (*ptr == '-')
    ++ptr;

  strmake(option_name_buf, ptr, MAX_OPTION_LEN + 1);

in server-tools/instance-manager/listener.cc create_unix_socket:
  strmake(unix_socket_address.sun_path, Options::Main::socket_file_name,
          sizeof(unix_socket_address.sun_path));

in server-tools/instance-manager/options.cc setup_windows_defaults:
  char base_name[FN_REFLEN];
  char im_name[FN_REFLEN];
  strmake(base_name, base_name_ptr, FN_REFLEN);
  *base_name_ptr= 0;

  strmake(im_name, base_name, FN_REFLEN);

in server-tools/instance-manager/user_map.cc User::User:
  user_length= (uint8) (strmake(user, user_name_arg->str,
                                USERNAME_LENGTH + 1) - user);
from user_map.h:
  char user[USERNAME_LENGTH + 1];

in sql/log.cc MYSQL_LOG::generate_name:
    strmake(buff, log_name, min(length, FN_REFLEN));
(open_slow_log:
    char buf[FN_REFLEN];
    return open(generate_name(log_name, ".log", 0, buf), LOG_NORMAL, 0,
                WRITE_CACHE);
)
And the same for open-query_log...

in sql/log.cc MYSQL_BIN_LOG::purge_logs_before_date:
char to_log[FN_REFLEN];
        strmake(to_log,
                log_info.log_file_name,
                sizeof(log_info.log_file_name));
typedef struct st_log_info
{
  char log_file_name[FN_REFLEN];

Just looking at get_object in sql/repl_failsafe.cc i got scared :) (ok saves some lines, but no beauty at all) but correct.

in sql/rpl_mi.cc init_master_info_with_options:
  if (master_password)
    strmake(mi->password, master_password, MAX_PASSWORD_LENGTH);
  
in sql/sql_acl.cc acl_getroot and acl_getroot_no_password:
    if (acl_user->host.hostname)
      strmake(sctx->priv_host, acl_user->host.hostname, MAX_HOSTNAME);
  
Hmm STRING_BUFFER_USUAL_SIZE, lack of imagination when brainstorming for name?
in sql/sql_plugin.cc check_func_set:
      strmake(buff, error, min(sizeof(buff), error_len));

in sql/sql_table.cc mysql_prepare_create_table:
      char tmp[64];
      strmake(strmake(tmp, save_cs->csname, sizeof(tmp)-4),
              STRING_WITH_LEN("_bin"));
  
in sql-common/client.c...

in strings/ctype-tis620.c my_strnxfrm_tis620:
  len= (size_t) (strmake((char*) dest, (char*) src, min(len, srclen)) -
                 (char*) dest);

I have not looked at the assembler code for strmake...
[25 Nov 2009 22:57] Sveta Smirnova
Thank you for the report.

Verified as described.

To repeat:

add to strmake call output of n to error log:  fprintf(stderr, "N: %d\n", n);

$bzr diff
=== modified file 'strings/strmake.c'
--- strings/strmake.c   2009-07-16 13:17:47 +0000
+++ strings/strmake.c   2009-11-25 22:49:10 +0000
@@ -50,5 +50,6 @@
     if (! (*dst++ = *src++))
       return dst-1;
   *dst=0;
+   fprintf(stderr, "N: %d\n", n);
   return dst;
 }

Run test partition_innodb_semi_consistent , save file var/log/mysqld.1.err

Modify strmake so it revert the patch for bug #46042:

=== modified file 'strings/strmake.c'
--- strings/strmake.c   2009-07-16 13:17:47 +0000
+++ strings/strmake.c   2009-11-25 22:38:09 +0000
@@ -41,14 +41,20 @@
     write a character rather than '\0' as this makes spotting these
     problems in the results easier.
   */
+       /*
   uint n= 0;
   while (n < length && src[n++]);
   memset(dst + n, (int) 'Z', length - n + 1);
+  */
+       uint n= strlen(src) + 1;
+       if (n <= length)
+               memset(dst + n, (int) 'Z', length - n + 1);
 #endif
 
   while (length--)
     if (! (*dst++ = *src++))
       return dst-1;
   *dst=0;
+  fprintf(stderr, "N: %d\n", n);
   return dst;
 }

Run test partition_innodb_semi_consistent, then compare saved mysqld.1.err and new one. You will see values of n would be different. Then compare if all strmake calls changed after fix for bug #46042 was applied. They are not.
[17 Dec 2009 17:59] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/94822

2824 Davi Arnaut	2009-12-17
      Bug#48983: Bad strmake calls (length one too long)
      
      The problem is a somewhat common misusage of the strmake function.
      The strmake(dst, src, len) function writes at most /len/ bytes to
      the string pointed to by src, not including the trailing null byte.
      Hence, if /len/ is the exact length of the destination buffer, a
      one byte buffer overflow can occur if the length of the source
      string is equal to or greater than /len/.
     @ client/mysqldump.c
        Make room for the trailing null byte.
     @ libmysql/libmysql.c
        Add comment, there is enough room in the buffer.
        Increase buffer length, two strings are concatenated.
     @ libmysqld/lib_sql.cc
        Make room for the trailing null byte.
     @ mysys/default.c
        Make room for the trailing null bytes.
     @ mysys/mf_pack.c
        Make room for the trailing null byte.
     @ server-tools/instance-manager/commands.cc
        Copy only if overflow isn't possible in both cases.
     @ server-tools/instance-manager/listener.cc
        Make room for the trailing null byte.
     @ sql/log.cc
        Make room for the trailing null byte.
     @ sql/sp_pcontext.h
        Cosmetic fix.
     @ sql/sql_acl.cc
        MAX_HOSTNAME already specifies space for the trailing null byte.
     @ sql/sql_parse.cc
        Make room for the trailing null byte.
     @ sql/sql_table.cc
        Make room for the trailing null byte.
[18 Dec 2009 19:14] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/94997

3286 Davi Arnaut	2009-12-18
      Bug#48983: Bad strmake calls (length one too long)
      
      MySQL 5.1 specific fixes.
[18 Dec 2009 19:16] Davi Arnaut
Queued to mysql-5.0-bugteam and up.
[14 Jan 2010 8:26] Bugs System
Pushed into 5.0.90 (revid:joro@sun.com-20100114082402-05fod2h6z9x9wok8) (version source revid:davi.arnaut@sun.com-20091217175838-pzt20wyp63immpzl) (merge vers: 5.0.89) (pib:16)
[15 Jan 2010 9:01] Bugs System
Pushed into 5.1.43 (revid:joro@sun.com-20100115085139-qkh0i0fpohd9u9p5) (version source revid:davi.arnaut@sun.com-20091218191409-gdgq8s7ctl6twd01) (merge vers: 5.1.42) (pib:16)
[22 Jan 2010 1:24] Paul Dubois
Noted in 5.0.90, 5.1.43 changelogs.

Several strmake() calls had an incorrect length argument (too large
by one). 

Setting report to NDI pending push to 5.5.x+.
[5 Feb 2010 11:48] Bugs System
Pushed into mysql-next-mr (revid:alik@sun.com-20100204063540-9czpdmpixi3iw2yb) (version source revid:alik@sun.com-20091224075613-es9uswo4lidkm3tj) (pib:16)
[5 Feb 2010 11:55] Bugs System
Pushed into 6.0.14-alpha (revid:alik@sun.com-20100205113942-oqovjy0eoqbarn7i) (version source revid:alik@sun.com-20100204064210-ljwanqvrjs83s1gq) (merge vers: 6.0.14-alpha) (pib:16)
[5 Feb 2010 12:01] Bugs System
Pushed into 5.5.2-m2 (revid:alik@sun.com-20100203172258-1n5dsotny40yufxw) (version source revid:alexey.kopytov@sun.com-20091223134205-pk9yvgfvpn3hy7lh) (merge vers: 5.5.1-m2) (pib:16)
[5 Feb 2010 17:11] Paul Dubois
Noted in 5.5.2, 6.0.14 changelogs.

Setting report to Need Merge pending push to Celosia.
[12 Mar 2010 14:15] Bugs System
Pushed into 5.1.44-ndb-7.0.14 (revid:jonas@mysql.com-20100312135944-t0z8s1da2orvl66x) (version source revid:jonas@mysql.com-20100312115609-woou0te4a6s4ae9y) (merge vers: 5.1.44-ndb-7.0.14) (pib:16)
[12 Mar 2010 14:31] Bugs System
Pushed into 5.1.44-ndb-6.2.19 (revid:jonas@mysql.com-20100312134846-tuqhd9w3tv4xgl3d) (version source revid:jonas@mysql.com-20100312060623-mx6407w2vx76h3by) (merge vers: 5.1.44-ndb-6.2.19) (pib:16)
[12 Mar 2010 14:47] Bugs System
Pushed into 5.1.44-ndb-6.3.33 (revid:jonas@mysql.com-20100312135724-xcw8vw2lu3mijrhn) (version source revid:jonas@mysql.com-20100312103652-snkltsd197l7q2yg) (merge vers: 5.1.44-ndb-6.3.33) (pib:16)
[12 Mar 2010 16:47] Paul Dubois
Fixed in earlier 5.1.x, 5.5.x.