Segmentation fault in myrocks::Rdb_string_reader::read

[george-lorch]

Reproducible at 414870a

Simple test case:

CREATE TABLE `t1` (
`a` bigint(20) NOT NULL,
`b` varchar(10) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL,
`u` bigint(20) unsigned NOT NULL,
`d` bigint(20) DEFAULT NULL,
PRIMARY KEY (`a`,`b`),
KEY `d` (`d`)
) ENGINE=ROCKSDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin COMMENT='ttl_duration=1000;ttl_col=u';
INSERT INTO t1 VALUES (100, 'aaabbb', UNIX_TIMESTAMP(), 200);
EXPLAIN SELECT COUNT(*) FROM t1;
SELECT COUNT(*) FROM t1;

And the backtrace. It seems that nullptr is being passed into Rdb_key_def::unpack_field for the unp_reader from Rdb_key_def::unpack_record but Rdb_key_def::unpack_binary_or_utf8_varchar_space_pad is taking a hint from fpi->m_unpack_info_uses_two_bytes that there should be an unp_reader present.

0x0000555556beba9a in myrocks::Rdb_string_reader::read (this=0x0, size=@0x7fffe87f4bfc: 1) at /home/glorch/dev/ps/facebook-mysql/storage/rocksdb/././rdb_buff.h:253
253 if (m_len < size) {
(gdb) p this
$1 = (myrocks::Rdb_string_reader * const) 0x0
(gdb) bt
#0 0x0000555556beba9a in myrocks::Rdb_string_reader::read (this=0x0, size=@0x7fffe87f4bfc: 1)
at /home/glorch/dev/ps/facebook-mysql/storage/rocksdb/././rdb_buff.h:253
#1 0x0000555556c4547f in myrocks::Rdb_string_reader::read_uint8 (this=0x0, res=0x7fffe87f4c54)
at /home/glorch/dev/ps/facebook-mysql/storage/rocksdb/./././rdb_buff.h:265
#2 0x0000555556c3934f in myrocks::Rdb_key_def::unpack_binary_or_utf8_varchar_space_pad (this=0x7fffbc0179b0, fpi=0x7fffbc013690,
field=0x7fffbc017258, dst=0x7fffbc01713a "aaabbb", '\217' <repeats 24 times>, "\366T\333[", reader=0x7fffe87f4dc0,
unp_reader=0x0) at /home/glorch/dev/ps/facebook-mysql/storage/rocksdb/rdb_datadic.cc:2458
#3 0x0000555556c36fd8 in myrocks::Rdb_key_def::unpack_field (this=0x7fffbc0179b0, fpi=0x7fffbc013690, field=0x7fffbc017258,
reader=0x7fffe87f4dc0, default_value=0x7fffbc0103d1 "", unp_reader=0x0)
at /home/glorch/dev/ps/facebook-mysql/storage/rocksdb/rdb_datadic.cc:1382
#4 0x0000555556c37595 in myrocks::Rdb_key_def::unpack_record (this=0x7fffbc0179b0, table=0x7fffbc013ed0,
buf=0x7fffbc017130 "\376d", packed_key=0x7fffe87f4e60, unpack_info=0x7fffe87f4e70,
verify_row_debug_checksums=@0x7fffbc014e89: false) at /home/glorch/dev/ps/facebook-mysql/storage/rocksdb/rdb_datadic.cc:1487
#5 0x0000555556bcd2a5 in myrocks::ha_rocksdb::secondary_index_read (this=0x7fffbc014830, keyno=1, buf=0x7fffbc017130 "\376d")
at /home/glorch/dev/ps/facebook-mysql/storage/rocksdb/ha_rocksdb.cc:7927
#6 0x0000555556bcfd50 in myrocks::ha_rocksdb::index_next_with_direction (this=0x7fffbc014830, buf=0x7fffbc017130 "\376d",
move_forward=true) at /home/glorch/dev/ps/facebook-mysql/storage/rocksdb/ha_rocksdb.cc:8663
#7 0x0000555556bd0129 in myrocks::ha_rocksdb::index_first_intern (this=0x7fffbc014830, buf=0x7fffbc017130 "\376d")
at /home/glorch/dev/ps/facebook-mysql/storage/rocksdb/ha_rocksdb.cc:8769
#8 0x0000555556bcfe38 in myrocks::ha_rocksdb::index_first (this=0x7fffbc014830, buf=0x7fffbc017130 "\376d")
at /home/glorch/dev/ps/facebook-mysql/storage/rocksdb/ha_rocksdb.cc:8680
#9 0x0000555556273f8a in handler::ha_index_first (this=0x7fffbc014830, buf=0x7fffbc017130 "\376d")
at /home/glorch/dev/ps/facebook-mysql/sql/handler.cc:3283
#10 0x000055555645dff8 in join_read_first (tab=0x7fffbc0071d8) at /home/glorch/dev/ps/facebook-mysql/sql/sql_executor.cc:2579
#11 0x000055555645b06b in sub_select (join=0x7fffbc0064f0, join_tab=0x7fffbc0071d8, end_of_records=false)
at /home/glorch/dev/ps/facebook-mysql/sql/sql_executor.cc:1297
#12 0x000055555645a8a9 in do_select (join=0x7fffbc0064f0) at /home/glorch/dev/ps/facebook-mysql/sql/sql_executor.cc:953
#13 0x00005555564586a1 in JOIN::exec (this=0x7fffbc0064f0) at /home/glorch/dev/ps/facebook-mysql/sql/sql_executor.cc:207
#14 0x00005555564cda23 in mysql_execute_select (thd=0x5555583d45f0, select_lex=0x5555583d8498, free_join=true)
at /home/glorch/dev/ps/facebook-mysql/sql/sql_select.cc:1133
#15 0x00005555564cdd59 in mysql_select (thd=0x5555583d45f0, tables=0x7fffbc005e88, wild_num=0, fields=..., conds=0x0,
order=0x5555583d8660, group=0x5555583d8598, having=0x0, select_options=2147748608, result=0x7fffbc0064c8, unit=0x5555583d7e40,
select_lex=0x5555583d8498) at /home/glorch/dev/ps/facebook-mysql/sql/sql_select.cc:1254
#16 0x00005555564cb9c2 in handle_select (thd=0x5555583d45f0, result=0x7fffbc0064c8, setup_tables_done_option=0)
at /home/glorch/dev/ps/facebook-mysql/sql/sql_select.cc:116
#17 0x000055555649c37a in execute_sqlcom_select (thd=0x5555583d45f0, all_tables=0x7fffbc005e88, last_timer=0x7fffe87f6878)
at /home/glorch/dev/ps/facebook-mysql/sql/sql_parse.cc:6503
#18 0x0000555556493b58 in mysql_execute_command (thd=0x5555583d45f0, statement_start_time=0x7fffe87f6718,
post_parse=0x7fffe87f6878) at /home/glorch/dev/ps/facebook-mysql/sql/sql_parse.cc:3663
#19 0x000055555649f5fe in mysql_parse (thd=0x5555583d45f0, rawbuf=0x7fffbc005b70 "select count(*) from test", length=25,
parser_state=0x7fffe87f7170, last_timer=0x7fffe87f6878, async_commit=0x7fffe87f6843 "")
at /home/glorch/dev/ps/facebook-mysql/sql/sql_parse.cc:7895
#20 0x000055555648f190 in dispatch_command (command=COM_QUERY, thd=0x5555583d45f0, packet=0x5555584b92f1 "", packet_length=25)
at /home/glorch/dev/ps/facebook-mysql/sql/sql_parse.cc:1919
#21 0x000055555648d01a in do_command (thd=0x5555583d45f0) at /home/glorch/dev/ps/facebook-mysql/sql/sql_parse.cc:1193
#22 0x00005555564478eb in do_handle_one_connection (thd_arg=0x5555583d45f0)
at /home/glorch/dev/ps/facebook-mysql/sql/sql_connect.cc:1148
#23 0x00005555564472e0 in handle_one_connection (arg=0x5555583d45f0) at /home/glorch/dev/ps/facebook-mysql/sql/sql_connect.cc:969
#24 0x0000555556bacd44 in pfs_spawn_thread (arg=0x5555583c86f0)
at /home/glorch/dev/ps/facebook-mysql/storage/perfschema/pfs.cc:1860
#25 0x00007ffff660c6db in start_thread (arg=0x7fffe87f8700) at pthread_create.c:463
#26 0x00007ffff59f688f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

[TenderWang]

I think that the code don't check the record has ttl , if it is TTL record , the code don't skip 8 byte TTL value in unpack_record function.

[TenderWang]

[TenderWang]

I modify code like this, and the problem can be solved. But I don't sure the code may lead to other problem.

[lth]

@LoveTommy Your fix is basically correct :)

However, I'd probably put the check in ha_rocksdb::secondary_index_read to match how it's done in ha_rocksdb::convert_record_from_storage_format:

mysql-5.6/storage/rocksdb/ha_rocksdb.cc

Lines 6104 to 6112 in 65d61c9

	/* If it's a TTL record, skip the 8 byte TTL value */
	const char *ttl_bytes;
	if (m_pk_descr->has_ttl()) {
	if ((ttl_bytes = reader.read(ROCKSDB_SIZEOF_TTL_RECORD))) {
	memcpy(m_ttl_bytes, ttl_bytes, ROCKSDB_SIZEOF_TTL_RECORD);
	} else {
	return HA_ERR_ROCKSDB_CORRUPT_DATA;
	}
	}

The idea is that unpack_record should only care about unpack_info in the "value" that is passed in, and nothing else.

[lth]

Actually, I think this code was meant to skip the ttl bytes for secondary keys:

mysql-5.6/storage/rocksdb/rdb_datadic.cc

Lines 1420 to 1429 in 65d61c9

	const bool has_unpack_info =
	unp_reader.remaining_bytes() && is_unpack_data_tag(unpack_header[0]);
	if (has_unpack_info) {
	if ((m_index_type == INDEX_TYPE_SECONDARY &&
	m_total_index_flags_length > 0 &&
	!unp_reader.read(m_total_index_flags_length)) ||
	!unp_reader.read(get_unpack_header_size(unpack_header[0]))) {
	return HA_ERR_ROCKSDB_CORRUPT_DATA;
	}
	}

We just need to move populating has_unpack_info after we skip the bytes, and replace if (has_unpack_info) with if (unp_reader.remaining_bytes()).

[TenderWang]

@lth 👍 Your idea is better than me. According to your idea, I try this in Rdb_key_def::unpack_record :

[lth]

@LoveTommy Would you mind submitting a PR with that fix (with mtr test as well)?

[george-lorch]

@lth and @LoveTommy I am already putting one together and testing right now with ^^ these exact changes and a test.

[george-lorch]

#898
Passed mtr locally