Bug#55077: Assertion failed: width > 0 && to != ((void *)0),

Alexey Kopytov · Alexey Kopytov · commit df389d0135d6 · 2010-08-25T19:57:53.000+04:00
file .\dtoa.c

The assertion failure was correct because the 'width' argument
of my_gcvt() has the signed integer type, whereas the unsigned
value UINT_MAX32 was being passed by the caller
(Field_double::val_str()) leading to a negative width in
my_gcvt().

The following chain of problems was found by further analysis:

1. The display width for a floating point number is calculated
in Field_double::val_str() as either field_length or the
maximum possible length of string representation of a floating
point number, whichever is greater. Since in the bug's test
case field_length is UINT_MAX32, we get the same value as the
display width. This does not make any sense because for numeric
values field_length only matters for ZEROFILL columns,
otherwise it does not make sense to allocate that much memory
just to print a number. Field_float::val_str() has a similar
problem.

2. Even if the above wasn't the case, we would still get a
crash on a slightly different test case when trying to allocate
UINT_MAX32 bytes with String::alloc() because the latter does
not handle such large input values correctly due to alignment
overflows.

3. Even when String::alloc() is fixed to return an error when
an alignment overflow occurs, there is still a problem because
almost no callers check its return value, and
Field_double::val_str() is not an exception (same for
Field_float::val_str()).

4. Even if all of the above wasn't the case, creating a
Field_double object with UINT_MAX32 as its field_length does
not make much sense either, since the .frm code limits it to
MAX_FIELD_CHARLENGTH (255) bytes. Such a beast can only be
created by create_tmp_field_from_item() from an Item with
REAL_RESULT as its result_type() and UINT_MAX32 as its
max_length.

5. For the bug's test case, the above condition (REAL_RESULT
Item with max_length = UINT_MAX32) was a result of
Item_func_if::fix_length_and_dec() "shortcutting" aggregation
of argument types when one of the arguments was a constant
NULL. In this case, the attributes of the aggregated type were
simply copied from the other, non-NULL argument, but max_length
was still calculated as per the general, non-shortcut case, by
choosing the greatest of argument's max_length, which is
obviously not correct.

The patch addresses all of the above problems, even though
fixing the assertion failure for the particular test case would
require only a subset of the above problems to be solved.
diff --git a/client/sql_string.cc b/client/sql_string.cc
@@ -31,9 +31,12 @@
 ** String functions
 *****************************************************************************/
 
-bool String::real_alloc(uint32 arg_length)
+bool String::real_alloc(uint32 length)
 {
-  arg_length=ALIGN_SIZE(arg_length+1);
+  uint32 arg_length= ALIGN_SIZE(length + 1);
+  DBUG_ASSERT(arg_length > length);
+  if (arg_length <= length)
+    return TRUE;                                 /* Overflow */
   str_length=0;
   if (Alloced_length < arg_length)
   {
@@ -56,6 +59,9 @@ bool String::real_alloc(uint32 arg_length)
 bool String::realloc(uint32 alloc_length)
 {
   uint32 len=ALIGN_SIZE(alloc_length+1);
+  DBUG_ASSERT(len > alloc_length);
+  if (len <= alloc_length)
+    return TRUE;                                 /* Overflow */
   if (Alloced_length < len)
   {
     char *new_ptr;
diff --git a/mysql-test/r/func_if.result b/mysql-test/r/func_if.result
@@ -186,3 +186,13 @@ MAX(IFNULL(CAST(c AS UNSIGNED), 0))
 12345678901234567890
 DROP TABLE t1;
 End of 5.0 tests
+#
+# Bug#55077: Assertion failed: width > 0 && to != ((void *)0), file .\dtoa.c
+# 
+CREATE TABLE t1 (a LONGBLOB, b DOUBLE);
+INSERT INTO t1 VALUES (NULL, 0), (NULL, 1);
+SELECT IF(b, (SELECT a FROM t1 LIMIT 1), b) c FROM t1 GROUP BY c;
+c
+NULL
+0
+DROP TABLE t1;
diff --git a/mysql-test/t/func_if.test b/mysql-test/t/func_if.test
@@ -165,3 +165,15 @@ DROP TABLE t1;
 
 
 --echo End of 5.0 tests
+
+
+--echo #
+--echo # Bug#55077: Assertion failed: width > 0 && to != ((void *)0), file .\dtoa.c
+--echo # 
+
+CREATE TABLE t1 (a LONGBLOB, b DOUBLE);
+INSERT INTO t1 VALUES (NULL, 0), (NULL, 1);
+
+SELECT IF(b, (SELECT a FROM t1 LIMIT 1), b) c FROM t1 GROUP BY c;
+
+DROP TABLE t1;
diff --git a/sql/field.cc b/sql/field.cc
@@ -4189,6 +4189,7 @@ String *Field_float::val_str(String *val_buffer,
 			     String *val_ptr __attribute__((unused)))
 {
   ASSERT_COLUMN_MARKED_FOR_READ;
+  DBUG_ASSERT(field_length <= MAX_FIELD_CHARLENGTH);
   float nr;
 #ifdef WORDS_BIGENDIAN
   if (table->s->db_low_byte_first)
@@ -4199,8 +4200,13 @@ String *Field_float::val_str(String *val_buffer,
 #endif
     memcpy(&nr, ptr, sizeof(nr));
 
-  uint to_length=max(field_length,70);
-  val_buffer->alloc(to_length);
+  uint to_length= 70;
+  if (val_buffer->alloc(to_length))
+  {
+    my_error(ER_OUT_OF_RESOURCES, MYF(0));
+    return val_buffer;
+  }
+
   char *to=(char*) val_buffer->ptr();
   size_t len;
 
@@ -4209,7 +4215,7 @@ String *Field_float::val_str(String *val_buffer,
   else
   {
     /*
-      We are safe here because the buffer length is >= 70, and
+      We are safe here because the buffer length is 70, and
       fabs(float) < 10^39, dec < NOT_FIXED_DEC. So the resulting string
       will be not longer than 69 chars + terminating '\0'.
     */
@@ -4506,6 +4512,7 @@ String *Field_double::val_str(String *val_buffer,
 			      String *val_ptr __attribute__((unused)))
 {
   ASSERT_COLUMN_MARKED_FOR_READ;
+  DBUG_ASSERT(field_length <= MAX_FIELD_CHARLENGTH);
   double nr;
 #ifdef WORDS_BIGENDIAN
   if (table->s->db_low_byte_first)
@@ -4515,9 +4522,13 @@ String *Field_double::val_str(String *val_buffer,
   else
 #endif
     doubleget(nr,ptr);
+  uint to_length= DOUBLE_TO_STRING_CONVERSION_BUFFER_SIZE;
+  if (val_buffer->alloc(to_length))
+  {
+    my_error(ER_OUT_OF_RESOURCES, MYF(0));
+    return val_buffer;
+  }
 
-  uint to_length=max(field_length, DOUBLE_TO_STRING_CONVERSION_BUFFER_SIZE);
-  val_buffer->alloc(to_length);
   char *to=(char*) val_buffer->ptr();
   size_t len;
 
diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
@@ -2560,27 +2560,30 @@ Item_func_if::fix_length_and_dec()
     cached_result_type= arg2_type;
     collation.set(args[2]->collation.collation);
     cached_field_type= args[2]->field_type();
+    max_length= args[2]->max_length;
+    return;
   }
-  else if (null2)
+
+  if (null2)
   {
     cached_result_type= arg1_type;
     collation.set(args[1]->collation.collation);
     cached_field_type= args[1]->field_type();
+    max_length= args[1]->max_length;
+    return;
+  }
+
+  agg_result_type(&cached_result_type, args + 1, 2);
+  if (cached_result_type == STRING_RESULT)
+  {
+    if (agg_arg_charsets_for_string_result(collation, args + 1, 2))
+      return;
   }
   else
   {
-    agg_result_type(&cached_result_type, args+1, 2);
-    if (cached_result_type == STRING_RESULT)
-    {
-      if (agg_arg_charsets_for_string_result(collation, args + 1, 2))
-        return;
-    }
-    else
-    {
-      collation.set_numeric(); // Number
-    }
-    cached_field_type= agg_field_type(args + 1, 2);
+    collation.set_numeric(); // Number
   }
+  cached_field_type= agg_field_type(args + 1, 2);
 
   uint32 char_length;
   if ((cached_result_type == DECIMAL_RESULT )
diff --git a/sql/sql_string.cc b/sql/sql_string.cc
@@ -31,9 +31,12 @@
 ** String functions
 *****************************************************************************/
 
-bool String::real_alloc(uint32 arg_length)
+bool String::real_alloc(uint32 length)
 {
-  arg_length=ALIGN_SIZE(arg_length+1);
+  uint32 arg_length= ALIGN_SIZE(length + 1);
+  DBUG_ASSERT(arg_length > length);
+  if (arg_length <= length)
+    return TRUE;                                 /* Overflow */
   str_length=0;
   if (Alloced_length < arg_length)
   {
@@ -56,6 +59,9 @@ bool String::real_alloc(uint32 arg_length)
 bool String::realloc(uint32 alloc_length)
 {
   uint32 len=ALIGN_SIZE(alloc_length+1);
+  DBUG_ASSERT(len > alloc_length);
+  if (len <= alloc_length)
+    return TRUE;                                 /* Overflow */
   if (Alloced_length < len)
   {
     char *new_ptr;

Original file line number	Diff line number	Diff line change
`@@ -31,9 +31,12 @@`
`31`	`31`	`** String functions`
`32`	`32`	`*****************************************************************************/`
`33`	`33`
`34`		`-bool String::real_alloc(uint32 arg_length)`
	`34`	`+bool String::real_alloc(uint32 length)`
`35`	`35`	`{`
`36`		`- arg_length=ALIGN_SIZE(arg_length+1);`
	`36`	`+ uint32 arg_length= ALIGN_SIZE(length + 1);`
	`37`	`+ DBUG_ASSERT(arg_length > length);`
	`38`	`+ if (arg_length <= length)`
	`39`	`+ return TRUE; /* Overflow */`
`37`	`40`	`str_length=0;`
`38`	`41`	`if (Alloced_length < arg_length)`
`39`	`42`	`{`
`@@ -56,6 +59,9 @@ bool String::real_alloc(uint32 arg_length)`
`56`	`59`	`bool String::realloc(uint32 alloc_length)`
`57`	`60`	`{`
`58`	`61`	`uint32 len=ALIGN_SIZE(alloc_length+1);`
	`62`	`+ DBUG_ASSERT(len > alloc_length);`
	`63`	`+ if (len <= alloc_length)`
	`64`	`+ return TRUE; /* Overflow */`
`59`	`65`	`if (Alloced_length < len)`
`60`	`66`	`{`
`61`	`67`	`char *new_ptr;`