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

[14 Sep 2006 15:08] Paul McCullagh
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().
[22 Sep 2006 11:04] Sveta Smirnova
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?
[22 Sep 2006 13:50] Paul McCullagh
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.
[23 Sep 2006 10:12] Sveta Smirnova
Thank you for the feedback.

Verified as described on Linux using last BK sources.
[30 Oct 2007 8:41] Bugs System
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.
[1 Nov 2007 11:26] Bugs System
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.
[7 Nov 2007 8:30] Bugs System
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.
[7 Nov 2007 8:33] Ingo Strüwing
Accidentally reset the status by modifying the reviewer flags in parallel to sending the patch.
[9 Nov 2007 18:43] Ingo Strüwing
Queued to 6.0-engines, 5.1-engines.
[14 Nov 2007 9:40] Bugs System
Pushed into 6.0.4-alpha
[14 Nov 2007 9:45] Bugs System
Pushed into 5.1.23-rc
[15 Nov 2007 10:37] Bugs System
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.
[15 Nov 2007 16:04] Ingo Strüwing
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.
[15 Nov 2007 16:53] Paul Dubois
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.
[28 Nov 2007 10:24] Bugs System
Pushed into 6.0.4-alpha
[28 Nov 2007 10:26] Bugs System
Pushed into 5.1.23-rc