MDEV-41179 MAX_KEY_PARTS ranges in select causing SEGV - #5782
mariadb-RexJohnston wants to merge 1 commit into
Conversation
751307b to
5e0b205
Compare
fecbad0 to
e90b83a
Compare
When a select containing 32 ranges is made on a table containing a
compound key with 32 parts, the range optimizer can run off the end
of a stack variable, invalidly overwriting subsequent stack variables.
In the struct st_sel_arg_range_seq, we have an array
RANGE_SEQ_ENTRY stack[MAX_REF_PARTS];
MAX_REF_PARTS is 32.
check_quick_select
/ sel_arg_range_seq_init
initialises stack[0] as NOT a key part
/ sel_arg_range_seq_next
iterates through the key parts, adding key part n to stack[n+1]
key part #32 gets referenced by step_down_to(), setting seq->i off the
end of the array.
Fix: RANGE_SEQ_ENTRY stack[MAX_REF_PARTS+1];
The above change exposed an issue with key length calculation on
MS Windows. Calling make_prev_keypart_map(32) caused the resultant
bitmap to be calculated as (1UL << 32) - 1. Using the MSVC compiler
this resulted in an empty key length calculation during
handler::index_read_map, causing an assertion in ha_innobase::index_read().
As we only need 32 bits to represent our key map, we change the type thus
-typedef ulong key_part_map;
+typedef uint32 key_part_map;
We correct make_keypart_map() and make_prev_keypart_map() to call our
overflow safe my_set_bits(). We also correct bka_range_seq_next()
and bkah_range_seq_next() to use make_prev_keypart_map().
We also add some DBUG_ASSERTS in key_part_map processing elsewhere,
exposing some issues in our BNLH implementation. We cap the number of
keyuse parts here, altering the explain output of 2 of our tests.
e90b83a to
7a40c0c
Compare
|
One more fix is needed, otherwise |
| { | ||
| return ((key_part_map)2 << (N)) - 1; | ||
| /* | ||
| my_set_bits() builds the mask in 64 bits and only then narrows it to |
There was a problem hiding this comment.
I suggest to remove this comment, it's redundant and not completely accurate
| { | ||
| return ((key_part_map)1 << (N)) - 1; | ||
| DBUG_ASSERT(N <= MAX_REF_PARTS); | ||
| return N ? (key_part_map) my_set_bits(N) : 0; |
There was a problem hiding this comment.
Worth commenting why we handle N==0 specially, in a way like "my_set_bits(0) is undefined as it shifts 1ULL by -1".
It's tempting to handle the N==0 case in my_set_bits but it's a behaviour change and apparently out of this commit scope. Currently my_set_bits(0) is UB and returns 0xffffffffffffffff, some call sites may not expect if it starts returning 0 instead.
| c16,c17,c18,c19,c20,c21,c22,c23,c24,c25,c26,c27,c28,c29,c30,c31) | ||
| ) ENGINE=InnoDB; | ||
|
|
||
| INSERT INTO t1 (c0) VALUES (1); |
There was a problem hiding this comment.
If one more value is inserted (INSERT INTO t1 (c0) VALUES (1),(2);) , then the issue is reproducible on the default MyISAM engine. I suggest doing this and moving the test into range.test as the bug is not InnoDB-specific.
| @@ -2170,7 +2170,7 @@ TEXT46 TEXT,TEXT47 TEXT,TEXT48 TEXT,TEXT49 TEXT,TEXT50 TEXT | |||
| EXPLAIN SELECT 1 FROM t1 NATURAL JOIN t1 AS t2; | |||
There was a problem hiding this comment.
Since the patch changes the logic of hash join, I think would be good to have not only EXPLAIN but the results of SELECT in the test too.
| keyuse++; | ||
| } while (keyuse->table == table && keyuse->key == key); | ||
| } while (keyuse->table == table && keyuse->key == key && | ||
| keyparts < keyinfo->usable_key_parts); |
There was a problem hiding this comment.
usable_key_parts counts only the leading non-HA_REVERSE_SORT parts. For PRIMARY KEY (p, a DESC) it is 1, so ref is built on (p) alone. The key is unique on (p, a), so a one-part ref looks like a unique lookup, the table is resolved as a const table, exactly one row is fetched for p=2. Repro:
CREATE TABLE t1 (p INT NOT NULL, a INT NOT NULL, b INT,
PRIMARY KEY (p, a DESC));
INSERT INTO t1 VALUES (1,1,1),(1,2,2),(2,1,3),(2,2,4);
SELECT * FROM t1 WHERE p=2 AND a=1; -- returns NOTHING (expected 2 1 3)
SELECT * FROM t1 IGNORE INDEX (PRIMARY) WHERE p=2 AND a=1; -- 2 1 3Is this bound needed here at all? If it is, then it probably should be keyparts < j->table->actual_n_key_parts(keyinfo). But keyparts for the synthetic $hj branch is already capped at MAX_REF_PARTS above, and for a real index keyuse->keypart is already bounded.
|
Other findings:
|
When a select containing 32 ranges is made on a table containing a compound key with 32 parts, the range optimizer can run off the end of a stack variable, invalidly overwriting subsequent stack variables.
In the struct st_sel_arg_range_seq, we have an array
RANGE_SEQ_ENTRY stack[MAX_REF_PARTS];
MAX_REF_PARTS is 32.
check_quick_select
/ sel_arg_range_seq_init
initialises stack[0] as NOT a key part
/ sel_arg_range_seq_next
iterates through the key parts, adding key part n to stack[n+1]
key part #32 gets referenced by step_down_to(), setting seq->i off the end of the array.
The above change has exposed an issue with key length calculation on MS Windows. The field is a ulong and with all 32 key parts wanted, we calculate this as (1UL << 32) - 1. The first part of the calculation overflows the field. The result is undefined in C++.
Fix: change key_part_map to 64 bits long, so it is the same on Linux
and MS Windows.