MySQL Bugs: #34749: Server crash when using NAME_CONST() with an aggregate function

Bug #34749	Server crash when using NAME_CONST() with an aggregate function
Submitted:	21 Feb 22:10	Modified:	15 Mar 22:17
Reporter:	David Shrewsbury
Status:	Closed
Category:	Server	Severity:	S1 (Critical)
Version:	5.0.54a, 5.1.23-rc	OS:	Linux
Assigned to:	Tatjana A. Nuernberg	Target Version:	5.0+
Tags:	name_const
Triage:	D1 (Critical)

Description:
Using NAME_CONST() with a negative number AND aggregate function causes a system crash.
This can be unavoidable in some cases of replication since the NAME_CONST() can be
inserted by the server (as in a stored procedure).

Note that this is similar to bug #30832, but this was tested on 5.0.54a which is supposed
to have the fix for that particular bug.

Backtrace follows:

(gdb) bt
#0  0x00002b3cbe72e422 in pthread_kill () from /lib/libpthread.so.0
#1  0x000000000072356f in write_core ()
#2  0x00000000005a8047 in handle_segfault ()
#3  <signal handler called>
#4  0x0000000000000000 in ?? ()
#5  0x00000000004db4f5 in Item::split_sum_func2 ()
#6  0x00000000005103b8 in Item_func::split_sum_func ()
#7  0x00000000005f0691 in setup_fields ()
#8  0x0000000000602a3b in JOIN::prepare ()
#9  0x0000000000602800 in mysql_select ()
#10 0x0000000000627f15 in handle_select ()
#11 0x00000000005bede4 in mysql_execute_command ()
#12 0x00000000005c4fcb in mysql_parse ()
#13 0x00000000005b9601 in dispatch_command ()
#14 0x00000000005b7438 in handle_one_connection ()
#15 0x00002b3cbe729317 in start_thread () from /lib/libpthread.so.0
#16 0x00002b3cbf5cbd5d in clone () from /lib/libc.so.6
#17 0x0000000000000000 in ?? ()

How to repeat:
DROP TABLE IF EXISTS t1;
CREATE TABLE t1 (a INT);

INSERT INTO t1 VALUES (1),(2),(3);

-- Must use a negative number in NAME_CONST() along with the aggregate
SELECT NAME_CONST('flag',-1) * MAX(a) FROM t1;

#0  Item_name_const::type (this=0x8d9c448) at item.cc:1222
#1  0x08181e17 in Item::split_sum_func2 (this=0x8d9c448, thd=0x8d642e8,
ref_pointer_array=0x8d9d738, fields=@0x8d9d678, ref=0x8d9c65c, skip_registered=true) at
item.cc:1308
#2  0x081b14f7 in Item_func::split_sum_func (this=0x8d9c608, thd=0x8d642e8,
ref_pointer_array=0x8d9d738, fields=@0x8d9d678) at item_func.cc:342
#3  0x08270957 in setup_fields (thd=0x8d642e8, ref_pointer_array=0x8d9d738,
fields=@0x8d65320, set_query_id=true, sum_func_list=0x8d9d678, allow_sum_func=true) at
sql_base.cc:5124
#4  0x082a48e4 in JOIN::prepare (this=0x8d9c938, rref_pointer_array=0x8d653b0,
tables_init=0x8d9c710, wild_num=0, conds_init=0x0, og_num=0, order_init=0x0,
group_init=0x0, having_init=0x0, proc_param_init=0x0, select_lex_arg=0x8d6528c,
unit_arg=0x8d65054) at sql_select.cc:467
#5  0x082a5723 in mysql_select (thd=0x8d642e8, rref_pointer_array=0x8d653b0,
tables=0x8d9c710, wild_num=0, fields=@0x8d65320, conds=0x0, og_num=0, order=0x0,
group=0x0, having=0x0, proc_param=0x0, select_options=2156153344, result=0x8d9c920,
unit=0x8d65054, select_lex=0x8d6528c) at sql_select.cc:2274
#6  0x082a9ba6 in handle_select (thd=0x8d642e8, lex=0x8d64ffc, result=0x8d9c920,
setup_tables_done_option=0) at sql_select.cc:257
#7  0x0823d89d in mysql_execute_command (thd=0x8d642e8) at sql_parse.cc:2736
#8  0x08245cde in mysql_parse (thd=0x8d642e8, inBuf=0x8d9c290 "SELECT
NAME_CONST('flag',-1)*max(1) FROM t1", length=43, found_semicolon=0xb50db1c0) at
sql_parse.cc:6174
#9  0x08248528 in dispatch_command (command=COM_QUERY, thd=0x8d642e8, packet=0x8d94231
"SELECT NAME_CONST('flag',-1)*max(1) FROM t1", packet_length=44) at sql_parse.cc:1889
#10 0x08249a81 in do_command (thd=0x8d642e8) at sql_parse.cc:1595
#11 0x0824ac5b in handle_one_connection (arg=0x8d642e8) at sql_parse.cc:1201

(gdb) print ((Item_func_neg *) value_item)->func_name()
$5 = 0x86353c7 "-"
(gdb) print ((Item_func_neg *) value_item)->val_int()
$6 = -1
(gdb) print ((Item_func_neg *) value_item)->basic_const_item()
$7 = true
(gdb) print ((Item_func_neg *) value_item)->args[0]
$8 = (class Item *) 0x8d9c370

(gdb) graph display *(((Item_func_neg *) this->value_item)->args[0]) 

(gdb) print (((Item_func_neg *) this->value_item)->args[0])->type()
$10 = INT_ITEM

This of course is "more fun with Item_func_neg" inasmuch as overloading like so unbreaks
the crash as such:

virtual enum Type type() const { return args[0]->type() == INT_ITEM ? INT_ITEM :
FUNC_ITEM; }

mysql> SELECT NAME_CONST('flag',-1) * MAX(a) FROM t1;
+--------------------------------+
| NAME_CONST('flag',-1) * MAX(a) |
+--------------------------------+
|                             -3 | 
+--------------------------------+

(note that above is just an illustration, not the actual fix as for instance doesn't
hotfix REAL_ITEM. fix in proper place.)

Fixing this in the same place as Bug#30832 in the name of consistency is a nice idea, but
(rather expectedly) affects execution plans. It is still interesting in that we get neg(1)
rather than -1 as an integer const in the first place or, to rephrase, that the item
identifies as FUNC and the func-type as UNKNOWN (implying at face value that to identify
neg() you'd look at the function's name(!!) instead, which I really won't comment on,
which in turn might lead one to conclude that since nobody introduced FUNC_NEG
identification, no such rewriting is taking place).

Hence, this is looks like a good idea (to the extent that it fixes in the same place that
the fix  for #30832 does), but ends up too broad:

class Item_func_neg :public Item_func_num1
{
  ...
  virtual enum Type type() const { return args[0]->basic_const_item() ? args[0]->type() :
Item_func_num1::type(); }
  ...

On the other hand, this only fixes the offending item-type "NAME_CONST" by making it
transparent where possible (that is, returns the underlying type of a item_func_neg()).
Which gives us, wait for it, this:

Item::Type Item_name_const::type() const
{
  ...
  /*
    valid_args guarantees value_item->basic_const_item(); if type is
    FUNC_ITEM, then we have a fudged item_func_neg() on our hands
    and return the underlying type.
  */
  return valid_args ?
           (value_item->type() == FUNC_ITEM) ?
             (((Item_func *) value_item)->key_item()->type() :
             value_item->type()) :
           NULL_ITEM;
}

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/43104

ChangeSet@1.2608, 2008-02-27 20:31:50+01:00, tnurnberg@mysql.com +3 -0
  Bug#34749: Server crash when using NAME_CONST() with an aggregate function
  
  NAME_CONST('whatever', -1) * MAX(whatever) bombed since -1 was
  not seen as constant, but as FUNCTION_UNARY_MINUS(constant)
  while we are at the same time pretending it was a basic const
  item. This confused the aggregate handlers in exciting ways.
  
  We now make NAME_CONST() behave more consistently in that when
  it is transparent with regard to basic_const_item(), it also
  is with regard to type().

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/43139

ChangeSet@1.2595, 2008-02-28 13:35:56+01:00, tnurnberg@mysql.com +5 -0
  Bug#34749: Server crash when using NAME_CONST() with an aggregate function
  
  NAME_CONST('whatever', -1) * MAX(whatever) bombed since -1 was
  not seen as constant, but as FUNCTION_UNARY_MINUS(constant)
  while we are at the same time pretending it was a basic const
  item. This confused the aggregate handlers in exciting ways.
  We now make NAME_CONST() behave more consistently.

REVIEWERS! : )

There are three ChangeSets attached. This is not evolution,
it is choice.

1 - Keep original patch for Bug#30832. Least intrusive code-wise,
    but a tad messy in terms of style.

2 - Rewrite Bug#30832 to apply more narrowly (only to NAME_CONST(),
    not to all unary minus). More elegant than 1, but uses funcname()
    to identify unary minus ("My func has no name!"  "How does it smell?"
    "AWFUL!!!")

3 - Like 2, but uses new identifier NEG_FUNC (as it should be). As a
    result also teaches ndb about this type because otherwise, we'll
    lose some rewrites, and execution plans will be affected.  Most
    stylish, but also most intrusive.

See all and decide which you'd like!  ; )
http://crazy.codetroop.com/randimg/?valfritt.jpg

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/43149

ChangeSet@1.2595, 2008-02-28 13:54:58+01:00, tnurnberg@mysql.com +7 -0
  Bug#34749: Server crash when using NAME_CONST() with an aggregate function
  
  NAME_CONST('whatever', -1) * MAX(whatever) bombed since -1 was
  not seen as constant, but as FUNCTION_UNARY_MINUS(constant)
  while we are at the same time pretending it was a basic const
  item. This confused the aggregate handlers in exciting ways.
  We now make NAME_CONST() behave more consistently.

Ok to push patch #3.

Tatjana, thanks for going extra kilometer to present all the options.

autopushing to 5.0.58, 5.1.24, 6.0.5 in opt

Pushed into 6.0.5-alpha

Pushed into 5.1.24-rc

Pushed into 5.0.60

Documented bugfix in the 5.0.60, 5.1.24, and 6.0.5 changelogs, as follows:

        Using NAME_CONST() with a negative number and an aggregate function
        caused MySQL to crash. This could also have a negative impact on
        replication.

Also noted in the 5.1.23-ndb-6.3.11 changelog.

Bug #36084 was marked as a duplicate of this one.