Uploaded image for project: 'Kudu'
  1. Kudu
  2. KUDU-2622

Validate read and write default value sizes when deserializing ColumnSchemaPB

    XMLWordPrintableJSON

Details

    • Bug
    • Status: Resolved
    • Critical
    • Resolution: Fixed
    • 1.8.0
    • 1.11.0
    • master, tablet
    • None

    Description

      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:

      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;
      

      Two mysteries remain:

      1. 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.
      2. 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:
        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}
        

      Attachments

        Issue Links

          Activity

            People

              oclarms Xu Yao
              adar Adar Dembo
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: