MySQL Bugs: #22351: handler::index_next_same() call to key_cmp_if

Bug #22351	handler::index_next_same() call to key_cmp_if_same() uses the wrong buffer
Submitted:	14 Sep 2006 15:08	Modified:	15 Nov 2007 16:53
Reporter:	Paul McCullagh (Basic Quality Contributor) (OCA)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server	Severity:	S2 (Serious)
Version:	5.1, BK Source	OS:	Linux (Linux)
Assigned to:	Ingo Strüwing	CPU Architecture:	Any
Tags:	bfsm_2007_10_18, handler, key_cmp_if_same, qc, wrong buffer

Description:
The function  "int handler::index_next_same(byte *buf, const byte *key, uint keylen)" makes the following call: key_cmp_if_same(table, key, active_index, keylen).

key_cmp_if_same() uses table->record[0]. Under circumstances, "table->record[0]" is not the same as "buf", passed to index_next_same().

In particular, this happens when ha_partition::handle_ordered_next() calls index_next_same(), using an engine that does not implement index_next_same().

How to repeat:
Comment out the ha_myisam implementation of index_next_same(). Then execute the following code:

drop table if exists t1;
CREATE TABLE t1 (a int, b int, primary key(a,b))
PARTITION BY KEY(b,a) PARTITIONS 2;
insert into t1 values (0,0),(1,1),(2,2),(3,3),(4,4),(5,5),(6,6);
select * from t1 where a = 4;

+---+---+
| a | b |
+---+---+
| 4 | 4 | 
| 6 | 6 | 
+---+---+

Suggested fix:
Pass the record buffer to be used into the function key_cmp_if_same().

Thank you for the report.

But it is not possible to simple "comment out the ha_myisam implementation of index_next_same()" without other changes using current BK sources. Could you please paste how you do it?

Although I originally repeated this error with the PBXT storage engine, I have been able to repeat the problem with MyISAM by doing the following.

In the file ha_myisam.cc rename the function 'index_next_same' to 'index_next_same_x'. Do the same for the declaration of index_next_same() in ha_myisam.h.

The statement "select * from t1 where a = 4;", will now return 2 rows instead of one. The reason is a bug in the implementation of handler:: index_next_same(), as described below.

Thank you for the feedback.

Verified as described on Linux using last BK sources.

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

ChangeSet@1.2598, 2007-10-30 09:40:50+01:00, istruewing@stella.local +1 -0
  Bug#22351 - handler::index_next_same() call to key_cmp_if_same() uses
              the wrong buffer
  
  handler::index_next_same() did not take into account that the
  internally called function key_cmp_if_same() uses the fixed
  buffer table->record[0] for key comparison instead of the
  buffer provided by the caller of handler::index_next_same().
  
  Added code to temporarily redirect table->record[0] to the
  record buffer provided by the caller of handler::index_next_same().
  
  The test case is in partition.test already.

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

ChangeSet@1.2598, 2007-11-01 12:26:14+01:00, istruewing@stella.local +1 -0
  Bug#22351 - handler::index_next_same() call to key_cmp_if_same() uses
              the wrong buffer
  
  handler::index_next_same() did not take into account that the
  internally called function key_cmp_if_same() uses the fixed
  buffer table->record[0] for key comparison instead of the
  buffer provided by the caller of handler::index_next_same().
  
  Added code to temporarily redirect table->record[0] to the
  record buffer provided by the caller of handler::index_next_same().
  
  The test case is in partition.test already.

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

ChangeSet@1.2598, 2007-11-07 09:30:41+01:00, istruewing@stella.local +1 -0
  Bug#22351 - handler::index_next_same() call to key_cmp_if_same() uses
              the wrong buffer
  
  handler::index_next_same() did not take into account that the
  internally called function key_cmp_if_same() uses the fixed
  buffer table->record[0] for key comparison instead of the
  buffer provided by the caller of handler::index_next_same().
  
  Added code to temporarily redirect table->record[0] and the fields
  used for the key to the record buffer provided by the caller of
  handler::index_next_same().
  
  The test case is in partition.test already.

Accidentally reset the status by modifying the reviewer flags in parallel to sending the patch.

Queued to 6.0-engines, 5.1-engines.

Pushed into 6.0.4-alpha

Pushed into 5.1.23-rc

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

ChangeSet@1.2618, 2007-11-15 11:36:50+01:00, istruewing@stella.local +2 -0
  Bug#22351 - handler::index_next_same() call to key_cmp_if_same() uses
              the wrong buffer
  Post-pushbuild fix
  Added test case for better coverage.

It is difficult to describe the user visible effect of this bug.

The most coarse description might be that one could select wrong data, where "select" does not only mean the SELECT statement, but possibly every statement that can have a WHERE clause.

Potentially affected were those storage engines, that do not redefine handler::index_next_same() and are capable of indexes. Namely all MySQL engines except MyISAM, MERGE, InnoDB, and CSV (Blackhole should also be safe as it cannot return wrong data). For example, I could repeat it with a MEMORY table.

However, handler::index_next_same() does not seem to be widely used. So the problem should be smaller than the corse description suggests. But I cannot give more information here. The test case from the bug report uses partitioning. But I doubt that this is a requirement for repeating the problem. If you need more details, I suggest we discuss it with the optimizer team.

Noted in 5.1.23, 6.0.4 changelogs.

For storage engines that do not redefine handler::index_next_same()
and are capable of indexes, statements that include a WHERE clause
might select incorrect data.

Pushed into 6.0.4-alpha

Pushed into 5.1.23-rc