MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:April 28 2010 8:11pm
Subject:bzr commit into mysql-6.0-codebase-bugfixing branch (guilhem:3845) Bug#52394
View as plain text  
#At file:///home/mysql_src/bzrrepos/mysql-6.0-codebase-bugfixing-50361-2/ based on revid:guilhem@stripped

 3845 Guilhem Bichot	2010-04-28
      Fix for BUG#52394 "Segfault in JOIN_CACHE::get_offset () at sql_select.h:445":
      code failed to see that there were no more records in the join buffer, so tried to read one more
      record, reading random data.
     @ sql/sql_join_cache.cc
        In the test's scenario, execution plan is D,C,E. Crash happened when reading a
        record from the last join buffer (of "(D JOIN C) JOIN E"). This is a JOIN_CACHE_BKA buffer,
        because primary key access will be used to find rows from E (see the join
        condition on E: "D.a = E.a", E.a is a primary key).
        In this JOIN_CACHE_BKA buffer, a record is encoded like this:
          length | pointer to pieces of previous table's JOIN_CACHE | fields :
        - "length" is the record's total length
        - "pointer" is because we are using "incremental buffers" (optimizer_join_cache_level=6).
        - "fields" is empty because fields from E can be found in the previous
        buffer (due to join condition). Thus the start of "fields" of record N
        is also the start of "length" of record N+1 (this matters below).
        JOIN_CACHE::last_record_pos remembers the position of the last record
        in the buffer (for "EOF" detection); more exactly, the position of the start of
        fields of the last record.
        When JOIN_CACHE_BKA::get_next_key() starts it compares "pos" (position right
        after the previously read record, i.e. position of the start of "length"
        of the maybe-existing to-be-read record) with last_rec_pos, to see if it is
        now at EOF.
        So when we have read the last record, and come to JOIN_CACHE_BKA::get_next_key()
        again, pos==last_rec_pos: pos>last_rec_pos is false, code believes that there
        is a record to read, though there isn't. It then tries to read some values in
        the record (random data), uses them to build a pointer (random pointer) to
        peek in the previous buffer (incremental buffers used), and causes
        a segmentation fault.
        The fix:
        1) compare apples to apples: "pos" is a start of length, "last_rec_pos"
        is a start of fields, this isn't the same: rather determine the position of the
        start of fields of the maybe-existing to-be-read record, and compare *that* to
        last_rec_pos: this gives a reliable EOF detection. This means that
        EOF <=> (pos + size_of_rec_len +
                  ((prev_cache != NULL) ? prev_cache->get_size_of_rec_offset() : 0))
                  > last_rec_pos                             (A)
        The same type of correct logic is found in JOIN_CACHE::get_record(): it builds
        the "start of fields" position of the maybe-existing record (pos+=size_of_rec_len etc),
        then calls read_all_record_fields() which compares this position with last_rec_pos.
        2) the correct inequality above can be simplified to
         pos >= last_record_pos      (B)
        Indeed:
        - (B) => (A) is clearly true.
        - !(B) => !(A) is true too because: if !(B), pos < last_record_pos.
        pos is the start of "length" of a thus _existing_ record.
        (pos + size_of_rec_len +
                  ((prev_cache != NULL) ? prev_cache->get_size_of_rec_offset() : 0))
        is the start of "fields" of this same record. By definition of last_rec_pos,
        this expression is thus smaller than or equal to last_rec_pos, so !(A) is true.
        3) we use (B) rather than (A) in code because when at an existing record it is
        the same amount of code, and when at EOF it is less code (a comparison, versus
        one or two additions and an if()).
        Why wasn't there any crash without incremental buffers: in that other case, E's "fields"
        are not empty as they don't exist in a previous buffer. So in get_next_key(),
        "pos" is "size of fields" bytes after last_rec_pos, ">" works, and function exits properly.

    modified:
      mysql-test/r/join_cache.result
      mysql-test/t/join_cache.test
      sql/sql_join_cache.cc
=== modified file 'mysql-test/r/join_cache.result'
--- a/mysql-test/r/join_cache.result	2010-03-17 12:25:15 +0000
+++ b/mysql-test/r/join_cache.result	2010-04-28 20:11:21 +0000
@@ -4168,3 +4168,18 @@ id	select_type	table	type	possible_keys	
 
 SET optimizer_join_cache_level=default;
 DROP TABLE t1,t2,t3,t4;
+#
+# BUG#52394 Segfault in JOIN_CACHE::get_offset () at sql_select.h:445
+#
+SET optimizer_join_cache_level = 6;
+CREATE TABLE C(a int);
+INSERT INTO C VALUES(1),(2),(3),(4),(5);
+CREATE TABLE D (a int(11), b varchar(1));
+INSERT INTO D VALUES (6,'r'),(27,'o');
+CREATE TABLE E (a int(11) primary key, b varchar(1));
+INSERT INTO E VALUES
+(14,'d'),(15,'z'),(16,'e'),(17,'h'),(18,'b'),(19,'s'),(20,'e'),(21,'j'),(22,'e'),(23,'f'),(24,'v'),(25,'x'),(26,'m'),(27,'c');
+SELECT 1 FROM C,D,E WHERE D.a = E.a AND D.b = E.b;
+1
+DROP TABLE C,D,E;
+SET optimizer_join_cache_level=default;

=== modified file 'mysql-test/t/join_cache.test'
--- a/mysql-test/t/join_cache.test	2010-03-17 12:25:15 +0000
+++ b/mysql-test/t/join_cache.test	2010-04-28 20:11:21 +0000
@@ -1824,3 +1824,23 @@ SELECT COUNT(*)
 SET optimizer_join_cache_level=default;
 
 DROP TABLE t1,t2,t3,t4;
+
+--echo #
+--echo # BUG#52394 Segfault in JOIN_CACHE::get_offset () at sql_select.h:445
+--echo #
+
+SET optimizer_join_cache_level = 6;
+
+CREATE TABLE C(a int);
+INSERT INTO C VALUES(1),(2),(3),(4),(5);
+
+CREATE TABLE D (a int(11), b varchar(1));
+INSERT INTO D VALUES (6,'r'),(27,'o');
+
+CREATE TABLE E (a int(11) primary key, b varchar(1));
+INSERT INTO E VALUES
+(14,'d'),(15,'z'),(16,'e'),(17,'h'),(18,'b'),(19,'s'),(20,'e'),(21,'j'),(22,'e'),(23,'f'),(24,'v'),(25,'x'),(26,'m'),(27,'c');
+
+SELECT 1 FROM C,D,E WHERE D.a = E.a AND D.b = E.b;
+DROP TABLE C,D,E;
+SET optimizer_join_cache_level=default;

=== modified file 'sql/sql_join_cache.cc'
--- a/sql/sql_join_cache.cc	2010-04-08 10:50:40 +0000
+++ b/sql/sql_join_cache.cc	2010-04-28 20:11:21 +0000
@@ -2448,7 +2448,11 @@ uint JOIN_CACHE_BKA::get_next_key(uchar 
   uchar *init_pos;
   JOIN_CACHE *cache;
   
-  if (pos > last_rec_pos || !records)
+  /*
+    '>=' (unlike '>' in JOIN_CACHE::record()) because we are not at fields'
+    start, and previous record's fields might be empty.
+  */
+  if (pos >= last_rec_pos || !records)
     return 0;
 
   /* Any record in a BKA cache is prepended with its length */


Attachment: [text/bzr-bundle] bzr/guilhem@mysql.com-20100428201121-3997h871x2hu1eee.bundle
Thread
bzr commit into mysql-6.0-codebase-bugfixing branch (guilhem:3845) Bug#52394Guilhem Bichot28 Apr