Bug #48983 Bad strmake calls (length one too long)

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.

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...

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.

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.

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.

Queued to mysql-5.0-bugteam and up.

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)

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)

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+.

Pushed into mysql-next-mr (revid:alik@sun.com-20100204063540-9czpdmpixi3iw2yb) (version source revid:alik@sun.com-20091224075613-es9uswo4lidkm3tj) (pib:16)

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)

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)

Noted in 5.5.2, 6.0.14 changelogs.

Setting report to Need Merge pending push to Celosia.

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)

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)

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)

Fixed in earlier 5.1.x, 5.5.x.