[ 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)