Bug#31752: check strmake() bounds

tnurnberg@mysql.com/white.intern.koehntopp.de · tnurnberg@mysql.com/white.intern.koehntopp.de · commit 1a95ed1df2bd · 2007-11-26T08:20:40.000+01:00
strmake() calls are easy to get wrong. Add checks in extra
debug mode to identify possible exploits.

Remove some dead code.

Remove some off-by-one errors identified with new checks.
diff --git a/sql/log.cc b/sql/log.cc
@@ -966,7 +966,7 @@ void MYSQL_LOG::make_log_name(char* buf, const char* log_ident)
   if (dir_len > FN_REFLEN)
     dir_len=FN_REFLEN-1;
   strnmov(buf, log_file_name, dir_len);
-  strmake(buf+dir_len, log_ident, FN_REFLEN - dir_len);
+  strmake(buf+dir_len, log_ident, FN_REFLEN - dir_len -1);
 }
 
 
diff --git a/sql/repl_failsafe.cc b/sql/repl_failsafe.cc
@@ -926,7 +926,7 @@ int load_master_data(THD* thd)
 			     0, (SLAVE_IO | SLAVE_SQL)))
           send_error(thd, ER_MASTER_INFO);
 	strmake(active_mi->master_log_name, row[0],
-		sizeof(active_mi->master_log_name));
+		sizeof(active_mi->master_log_name) -1);
 	active_mi->master_log_pos= my_strtoll10(row[1], (char**) 0, &error);
         /* at least in recent versions, the condition below should be false */
 	if (active_mi->master_log_pos < BIN_LOG_HEADER_SIZE)
diff --git a/sql/set_var.cc b/sql/set_var.cc
@@ -1573,7 +1573,7 @@ bool sys_var::check_set(THD *thd, set_var *var, TYPELIB *enum_names)
 					    &not_used));
     if (error_len)
     {
-      strmake(buff, error, min(sizeof(buff), error_len));
+      strmake(buff, error, min(sizeof(buff) - 1, error_len));
       goto err;
     }
   }
diff --git a/sql/sql_show.cc b/sql/sql_show.cc
@@ -136,7 +136,7 @@ int mysqld_show_tables(THD *thd,const char *db,const char *wild)
 {
   Item_string *field=new Item_string("",0,thd->charset());
   List<Item> field_list;
-  char path[FN_LEN],*end;
+  char path[FN_REFLEN],*end;
   List<char> files;
   char *file_name;
   Protocol *protocol= thd->protocol;
@@ -457,7 +457,7 @@ int mysqld_extend_show_tables(THD *thd,const char *db,const char *wild)
   Item *item;
   List<char> files;
   List<Item> field_list;
-  char path[FN_LEN];
+  char path[FN_REFLEN];
   char *file_name;
   TABLE *table;
   Protocol *protocol= thd->protocol;
diff --git a/sql/unireg.cc b/sql/unireg.cc
@@ -140,6 +140,9 @@ bool mysql_create_frm(THD *thd, my_string file_name,
   strmake((char*) forminfo+47,create_info->comment ? create_info->comment : "",
 	  60);
   forminfo[46]=(uchar) strlen((char*)forminfo+47);	// Length of comment
+#ifdef EXTRA_DEBUG
+  memset((char*) forminfo+47 + forminfo[46], 0, 61 - forminfo[46]);
+#endif
 
   if (my_pwrite(file,(byte*) fileinfo,64,0L,MYF_RW) ||
       my_pwrite(file,(byte*) keybuff,key_info_length,
diff --git a/strings/strmake.c b/strings/strmake.c
@@ -28,23 +28,25 @@
 #include <my_global.h>
 #include "m_string.h"
 
-#ifdef BAD_STRING_COMPILER
-
-char *strmake(char *dst,const char *src,uint length)
+char *strmake(register char *dst, register const char *src, uint length)
 {
-  reg1 char *res;
-
-  if ((res=memccpy(dst,src,0,length)))
-    return res-1;
-  dst[length]=0;
-  return dst+length;
-}
-
-#define strmake strmake_overlapp	/* Use orginal for overlapping str */
+#ifdef EXTRA_DEBUG
+  /*
+    'length' is the maximum length of the string; the buffer needs
+    to be one character larger to accomodate the terminating '\0'.
+    This is easy to get wrong, so we make sure we write to the
+    entire length of the buffer to identify incorrect buffer-sizes.
+    We only initialise the "unused" part of the buffer here, a) for
+    efficiency, and b) because dst==src is allowed, so initialising
+    the entire buffer would overwrite the source-string. Also, we
+    write a character rather than '\0' as this makes spotting these
+    problems in the results easier.
+  */
+  uint n= strlen(src) + 1;
+  if (n <= length)
+    memset(dst + n, (int) 'Z', length - n + 1);
 #endif
 
-char *strmake(register char *dst, register const char *src, uint length)
-{
   while (length--)
     if (! (*dst++ = *src++))
       return dst-1;

Original file line number	Diff line number	Diff line change
`@@ -966,7 +966,7 @@ void MYSQL_LOG::make_log_name(char* buf, const char* log_ident)`
`966`	`966`	`if (dir_len > FN_REFLEN)`
`967`	`967`	`dir_len=FN_REFLEN-1;`
`968`	`968`	`strnmov(buf, log_file_name, dir_len);`
`969`		`- strmake(buf+dir_len, log_ident, FN_REFLEN - dir_len);`
	`969`	`+ strmake(buf+dir_len, log_ident, FN_REFLEN - dir_len -1);`
`970`	`970`	`}`
`971`	`971`
`972`	`972`
Original file line number	Diff line number	Diff line change
`@@ -1573,7 +1573,7 @@ bool sys_var::check_set(THD thd, set_var var, TYPELIB *enum_names)`
`1573`	`1573`	`&not_used));`
`1574`	`1574`	`if (error_len)`
`1575`	`1575`	`{`
`1576`		`- strmake(buff, error, min(sizeof(buff), error_len));`
	`1576`	`+ strmake(buff, error, min(sizeof(buff) - 1, error_len));`
`1577`	`1577`	`goto err;`
`1578`	`1578`	`}`
`1579`	`1579`	`}`