[
https://issues.apache.org/jira/browse/KUDU-2622?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16680345#comment-16680345
]
Adar Dembo commented on KUDU-2622:
----------------------------------
{quote}
The thick clients use validation to prevent users from accidentally doing this,
yet for some reason TestKuduBackup.testRandomBackupAndRestore (in the
kudu-backup project) is able to repro this from time to time. I suspect it's
because it chooses certain values (such as a minimum DECIMAL128 value) that
result in nul bytes in the default value byte sequence, and then server-side
protobuf parses this into a string that terminates at the nul byte (again,
maybe this is due to a libc++ string detail).
{quote}
This turned out to be a separate issue: another instance of KUDU-2378. I'm
fixing that now; the lack of validation here though is still an issue.
> Validate read and write default value sizes when deserializing ColumnSchemaPB
> -----------------------------------------------------------------------------
>
> Key: KUDU-2622
> URL: https://issues.apache.org/jira/browse/KUDU-2622
> Project: Kudu
> Issue Type: Bug
> Components: master, tablet
> Affects Versions: 1.8.0
> Reporter: Adar Dembo
> Priority: Critical
>
> ColumnSchemaFromPB (see wire_protocol.cc) doesn't validate the size of the
> column's read_default or write_default values. Because of this, a specially
> crafted ColumnSchemaPB can crash either the master (creating a table or
> altering a table to add a column) or the tserver (writing, scanning with a
> deprecated ColumnRangePredicate, splitting a key range, and possibly more).
> Here's a patch that'll reliably trigger a crash in TSAN:
> {noformat}
> diff --git a/src/kudu/master/master-test.cc b/src/kudu/master/master-test.cc
> index 591644848..7bbc1d160 100644
> --- a/src/kudu/master/master-test.cc
> +++ b/src/kudu/master/master-test.cc
> @@ -787,6 +787,23 @@ TEST_F(MasterTest, TestCreateTableInvalidSchema) {
> SecureShortDebugString(resp.error().status()));
> }
>
> +TEST_F(MasterTest, TestCreateTableCrashReadDefault) {
> + CreateTableRequestPB req;
> + CreateTableResponsePB resp;
> + RpcController controller;
> +
> + req.set_name("table");
> + ColumnSchemaPB* col = req.mutable_schema()->add_columns();
> + col->set_name("col");
> + col->set_type(DECIMAL128);
> + col->set_is_key(true);
> + col->set_read_default_value("foo"); // too short!
> +
> + ASSERT_OK(proxy_->CreateTable(req, &resp, &controller));
> + SCOPED_TRACE(SecureDebugString(resp));
> + ASSERT_FALSE(resp.has_error());
> +}
> +
> TEST_F(MasterTest, TestVirtualColumns) {
> CreateTableRequestPB req;
> CreateTableResponsePB resp;
> {noformat}
> Two mysteries remain:
> # I can't repro the crash anywhere except in TSAN, which makes me think it's
> some interaction with libc++'s std::string implementation, and that
> libstdc++'s std::string doesn't trigger it for some reason. Though I suspect
> MSAN would also catch this, if we supported that.
> # The thick clients use validation to prevent users from accidentally doing
> this, yet for some reason TestKuduBackup.testRandomBackupAndRestore (in the
> kudu-backup project) is able to repro this from time to time. I suspect it's
> because it chooses certain values (such as a minimum DECIMAL128 value) that
> result in nul bytes in the default value byte sequence, and then server-side
> protobuf parses this into a string that terminates at the nul byte (again,
> maybe this is due to a libc++ string detail). For example:
> {noformat}
> TEST_F(MasterTest, TestCreateTableCrashReadDefault) {
> CreateTableRequestPB req;
> CreateTableResponsePB resp;
> RpcController controller;
> req.set_name("table");
> ColumnSchemaPB* col = req.mutable_schema()->add_columns();
> col->set_name("col");
> col->set_type(DECIMAL128);
> col->set_is_key(true);
> string default_val = "\001\000\000_\02231\344=,\377\377\377\377\377\377";
> col->set_read_default_value(std::move(default_val));
> ASSERT_OK(proxy_->CreateTable(req, &resp, &controller));
> SCOPED_TRACE(SecureDebugString(resp));
> ASSERT_FALSE(resp.has_error());
> }
> ...
> I1108 00:24:17.118602 27003 catalog_manager.cc:1373] Servicing CreateTable
> request from {username='adar'} at 127.0.0.1:57014:
> name: "table"
> schema {
> columns {
> name: "col"
> type: DECIMAL128
> is_key: true
> read_default_value: "\001"
> }
> }
> Thread 25 "rpc worker-2483" received signal SIGSEGV, Segmentation fault.
> 0x000000000051ff77 in kudu::Variant::Reset (this=0x7b0c0007c080,
> type=kudu::DECIMAL128,
> value=0x7b0800037101) at ../../src/kudu/common/types.h:706
> 706 numeric_.i128 = *static_cast<const int128_t *>(value);
> (gdb) up
> #1 0x000000000051fc83 in kudu::Variant::Variant (this=0x7b0c0007c080,
> type=kudu::UINT8,
> value=0x5ce40000025080) at ../../src/kudu/common/types.h:644
> 644 Reset(type, value);
> (gdb) up
> #2 0x0000000000518cb5 in kudu::ColumnSchema::ColumnSchema
> (this=0x7fffdc3b9650, name=...,
> type=kudu::DECIMAL128, is_nullable=false, read_default=0x7b0800037101,
> write_default=0x7b0800037121, attributes=..., type_attributes=...)
> at ../../src/kudu/common/schema.h:209
> 209 read_default_(read_default ? new Variant(type, read_default) :
> nullptr),
> (gdb) up
> #3 0x00007ffff333d5bc in kudu::ColumnSchemaFromPB (pb=...)
> at ../../src/kudu/common/wire_protocol.cc:291
> 291 return ColumnSchema(pb.name(), pb.type(), pb.is_nullable(),
> (gdb) list
> 286 attributes.compression = pb.compression();
> 287 }
> 288 if (pb.has_cfile_block_size()) {
> 289 attributes.cfile_block_size = pb.cfile_block_size();
> 290 }
> 291 return ColumnSchema(pb.name(), pb.type(), pb.is_nullable(),
> 292 read_default_ptr, write_default_ptr,
> 293 attributes, type_attributes);
> 294 }
> 295
> (gdb) p read_default
> $1 = {data_ = 0x7b0800037101 "\001", size_ = 1}
> {noformat}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
