kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From a...@apache.org
Subject [kudu] branch master updated: KUDU-2622 Validate read and write default value sizes when deserializing ColumnSchemaPB
Date Mon, 15 Jul 2019 21:53:08 GMT
This is an automated email from the ASF dual-hosted git repository.

adar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git


The following commit(s) were added to refs/heads/master by this push:
     new 192cf7d  KUDU-2622 Validate read and write default value sizes when deserializing
ColumnSchemaPB
192cf7d is described below

commit 192cf7df31503a6d2a5bffc54619ba239639891b
Author: oclarms <oclarms@gmail.com>
AuthorDate: Thu Jul 11 12:59:06 2019 +0800

    KUDU-2622 Validate read and write default value sizes when deserializing ColumnSchemaPB
    
    Fix the issue of out-of-bounds memory access caused by invalid read and write default
values.
    
    Change-Id: I434f2aac49402af4f29956ab20bba352dc719eeb
    Reviewed-on: http://gerrit.cloudera.org:8080/13842
    Tested-by: Kudu Jenkins
    Reviewed-by: Adar Dembo <adar@cloudera.com>
---
 src/kudu/common/wire_protocol-test.cc | 80 ++++++++++++++++++++++++-----------
 src/kudu/common/wire_protocol.cc      | 23 +++++++---
 src/kudu/common/wire_protocol.h       |  3 +-
 src/kudu/master/catalog_manager.cc    |  9 ++--
 src/kudu/tserver/tablet_service.cc    | 29 ++++++++-----
 5 files changed, 98 insertions(+), 46 deletions(-)

diff --git a/src/kudu/common/wire_protocol-test.cc b/src/kudu/common/wire_protocol-test.cc
index d2ad3cb..0317cab 100644
--- a/src/kudu/common/wire_protocol-test.cc
+++ b/src/kudu/common/wire_protocol-test.cc
@@ -487,42 +487,47 @@ TEST_F(WireProtocolTest, TestColumnDefaultValue) {
 
   ColumnSchema col1("col1", STRING);
   ColumnSchemaToPB(col1, &pb);
-  ColumnSchema col1fpb = ColumnSchemaFromPB(pb);
-  ASSERT_FALSE(col1fpb.has_read_default());
-  ASSERT_FALSE(col1fpb.has_write_default());
-  ASSERT_TRUE(col1fpb.read_default_value() == nullptr);
+  boost::optional<ColumnSchema> col1fpb;
+  ASSERT_OK(ColumnSchemaFromPB(pb, &col1fpb));
+  ASSERT_FALSE(col1fpb->has_read_default());
+  ASSERT_FALSE(col1fpb->has_write_default());
+  ASSERT_TRUE(col1fpb->read_default_value() == nullptr);
 
   ColumnSchema col2("col2", STRING, false, &read_default_str);
   ColumnSchemaToPB(col2, &pb);
-  ColumnSchema col2fpb = ColumnSchemaFromPB(pb);
-  ASSERT_TRUE(col2fpb.has_read_default());
-  ASSERT_FALSE(col2fpb.has_write_default());
-  ASSERT_EQ(read_default_str, *static_cast<const Slice *>(col2fpb.read_default_value()));
-  ASSERT_EQ(nullptr, static_cast<const Slice *>(col2fpb.write_default_value()));
+  boost::optional<ColumnSchema> col2fpb;
+  ASSERT_OK(ColumnSchemaFromPB(pb, &col2fpb));
+  ASSERT_TRUE(col2fpb->has_read_default());
+  ASSERT_FALSE(col2fpb->has_write_default());
+  ASSERT_EQ(read_default_str, *static_cast<const Slice *>(col2fpb->read_default_value()));
+  ASSERT_EQ(nullptr, static_cast<const Slice *>(col2fpb->write_default_value()));
 
   ColumnSchema col3("col3", STRING, false, &read_default_str, &write_default_str);
   ColumnSchemaToPB(col3, &pb);
-  ColumnSchema col3fpb = ColumnSchemaFromPB(pb);
-  ASSERT_TRUE(col3fpb.has_read_default());
-  ASSERT_TRUE(col3fpb.has_write_default());
-  ASSERT_EQ(read_default_str, *static_cast<const Slice *>(col3fpb.read_default_value()));
-  ASSERT_EQ(write_default_str, *static_cast<const Slice *>(col3fpb.write_default_value()));
+  boost::optional<ColumnSchema> col3fpb;
+  ASSERT_OK(ColumnSchemaFromPB(pb, &col3fpb));
+  ASSERT_TRUE(col3fpb->has_read_default());
+  ASSERT_TRUE(col3fpb->has_write_default());
+  ASSERT_EQ(read_default_str, *static_cast<const Slice *>(col3fpb->read_default_value()));
+  ASSERT_EQ(write_default_str, *static_cast<const Slice *>(col3fpb->write_default_value()));
 
   ColumnSchema col4("col4", UINT32, false, &read_default_u32);
   ColumnSchemaToPB(col4, &pb);
-  ColumnSchema col4fpb = ColumnSchemaFromPB(pb);
-  ASSERT_TRUE(col4fpb.has_read_default());
-  ASSERT_FALSE(col4fpb.has_write_default());
-  ASSERT_EQ(read_default_u32, *static_cast<const uint32_t *>(col4fpb.read_default_value()));
-  ASSERT_EQ(nullptr, static_cast<const uint32_t *>(col4fpb.write_default_value()));
+  boost::optional<ColumnSchema> col4fpb;
+  ASSERT_OK(ColumnSchemaFromPB(pb, &col4fpb));
+  ASSERT_TRUE(col4fpb->has_read_default());
+  ASSERT_FALSE(col4fpb->has_write_default());
+  ASSERT_EQ(read_default_u32, *static_cast<const uint32_t *>(col4fpb->read_default_value()));
+  ASSERT_EQ(nullptr, static_cast<const uint32_t *>(col4fpb->write_default_value()));
 
   ColumnSchema col5("col5", UINT32, false, &read_default_u32, &write_default_u32);
   ColumnSchemaToPB(col5, &pb);
-  ColumnSchema col5fpb = ColumnSchemaFromPB(pb);
-  ASSERT_TRUE(col5fpb.has_read_default());
-  ASSERT_TRUE(col5fpb.has_write_default());
-  ASSERT_EQ(read_default_u32, *static_cast<const uint32_t *>(col5fpb.read_default_value()));
-  ASSERT_EQ(write_default_u32, *static_cast<const uint32_t *>(col5fpb.write_default_value()));
+  boost::optional<ColumnSchema> col5fpb;
+  ASSERT_OK(ColumnSchemaFromPB(pb, &col5fpb));
+  ASSERT_TRUE(col5fpb->has_read_default());
+  ASSERT_TRUE(col5fpb->has_write_default());
+  ASSERT_EQ(read_default_u32, *static_cast<const uint32_t *>(col5fpb->read_default_value()));
+  ASSERT_EQ(write_default_u32, *static_cast<const uint32_t *>(col5fpb->write_default_value()));
 }
 
 // Regression test for KUDU-2378; the call to ColumnSchemaFromPB yielded a crash.
@@ -531,7 +536,32 @@ TEST_F(WireProtocolTest, TestCrashOnAlignedLoadOf128BitReadDefault) {
   pb.set_name("col");
   pb.set_type(DECIMAL128);
   pb.set_read_default_value(string(16, 'a'));
-  ColumnSchemaFromPB(pb);
+  boost::optional<ColumnSchema> col;
+  ASSERT_OK(ColumnSchemaFromPB(pb, &col));
+}
+
+// Regression test for KUDU-2622; Validate read and write default value sizes.
+TEST_F(WireProtocolTest, TestInvalidReadAndWriteDefault) {
+  {
+    ColumnSchemaPB pb;
+    pb.set_name("col");
+    pb.set_type(DECIMAL128);
+    pb.set_read_default_value(string(8, 'a'));
+    boost::optional<ColumnSchema> col;
+    Status s = ColumnSchemaFromPB(pb, &col);
+    EXPECT_TRUE(s.IsCorruption());
+    ASSERT_STR_CONTAINS(s.ToString(), "Corruption: Not enough bytes for decimal: read default");
+  }
+  {
+    ColumnSchemaPB pb;
+    pb.set_name("col");
+    pb.set_type(DECIMAL128);
+    pb.set_write_default_value(string(8, 'a'));
+    boost::optional<ColumnSchema> col;
+    Status s = ColumnSchemaFromPB(pb, &col);
+    EXPECT_TRUE(s.IsCorruption());
+    ASSERT_STR_CONTAINS(s.ToString(), "Corruption: Not enough bytes for decimal: write default");
+  }
 }
 
 TEST_F(WireProtocolTest, TestColumnPredicateInList) {
diff --git a/src/kudu/common/wire_protocol.cc b/src/kudu/common/wire_protocol.cc
index 50d2133..d40f813 100644
--- a/src/kudu/common/wire_protocol.cc
+++ b/src/kudu/common/wire_protocol.cc
@@ -257,7 +257,7 @@ void ColumnSchemaToPB(const ColumnSchema& col_schema, ColumnSchemaPB
*pb, int fl
   }
 }
 
-ColumnSchema ColumnSchemaFromPB(const ColumnSchemaPB& pb) {
+Status ColumnSchemaFromPB(const ColumnSchemaPB& pb, boost::optional<ColumnSchema>*
col_schema) {
   const void *write_default_ptr = nullptr;
   const void *read_default_ptr = nullptr;
   Slice write_default;
@@ -268,6 +268,11 @@ ColumnSchema ColumnSchemaFromPB(const ColumnSchemaPB& pb) {
     if (typeinfo->physical_type() == BINARY) {
       read_default_ptr = &read_default;
     } else {
+      if (typeinfo->size() > read_default.size()) {
+        return Status::Corruption(
+            Substitute("Not enough bytes for $0: read default size ($1) less than type size
($2)",
+                       typeinfo->name(), read_default.size(), typeinfo->size()));
+      }
       read_default_ptr = read_default.data();
     }
   }
@@ -276,6 +281,11 @@ ColumnSchema ColumnSchemaFromPB(const ColumnSchemaPB& pb) {
     if (typeinfo->physical_type() == BINARY) {
       write_default_ptr = &write_default;
     } else {
+      if (typeinfo->size() > write_default.size()) {
+        return Status::Corruption(
+            Substitute("Not enough bytes for $0: write default size ($1) less than type size
($2)",
+                       typeinfo->name(), write_default.size(), typeinfo->size()));
+      }
       write_default_ptr = write_default.data();
     }
   }
@@ -306,9 +316,10 @@ ColumnSchema ColumnSchemaFromPB(const ColumnSchemaPB& pb) {
   // in protobuf is the empty string. So, it's safe to use pb.comment() directly
   // regardless of whether has_comment() is true or false.
   // https://developers.google.com/protocol-buffers/docs/proto#optional
-  return ColumnSchema(pb.name(), pb.type(), pb.is_nullable(),
-                      read_default_ptr, write_default_ptr,
-                      attributes, type_attributes, pb.comment());
+  *col_schema = ColumnSchema(pb.name(), pb.type(), pb.is_nullable(),
+                             read_default_ptr, write_default_ptr,
+                             attributes, type_attributes, pb.comment());
+  return Status::OK();
 }
 
 void ColumnSchemaDeltaToPB(const ColumnSchemaDelta& col_delta, ColumnSchemaDeltaPB *pb)
{
@@ -373,7 +384,9 @@ Status ColumnPBsToSchema(const RepeatedPtrField<ColumnSchemaPB>&
column_pbs,
   int num_key_columns = 0;
   bool is_handling_key = true;
   for (const ColumnSchemaPB& pb : column_pbs) {
-    columns.push_back(ColumnSchemaFromPB(pb));
+    boost::optional<ColumnSchema> column;
+    RETURN_NOT_OK(ColumnSchemaFromPB(pb, &column));
+    columns.push_back(*column);
     if (pb.is_key()) {
       if (!is_handling_key) {
         return Status::InvalidArgument(
diff --git a/src/kudu/common/wire_protocol.h b/src/kudu/common/wire_protocol.h
index cbc7c60..e29c568 100644
--- a/src/kudu/common/wire_protocol.h
+++ b/src/kudu/common/wire_protocol.h
@@ -112,7 +112,8 @@ Status SchemaFromPB(const SchemaPB& pb, Schema *schema);
 void ColumnSchemaToPB(const ColumnSchema& schema, ColumnSchemaPB *pb, int flags = 0);
 
 // Return the ColumnSchema created from the specified protobuf.
-ColumnSchema ColumnSchemaFromPB(const ColumnSchemaPB& pb);
+// If the column schema is invalid, return a non-OK status.
+Status ColumnSchemaFromPB(const ColumnSchemaPB& pb, boost::optional<ColumnSchema>*
col_schema);
 
 // Convert the given list of ColumnSchemaPB objects into a Schema object.
 //
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index a2c4cd8..54b9592 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -2091,13 +2091,14 @@ Status CatalogManager::ApplyAlterSchemaSteps(const SysTablesEntryPB&
current_pb,
         RETURN_NOT_OK(ProcessColumnPBDefaults(&new_col_pb));
 
         // Can't accept a NOT NULL column without a default.
-        ColumnSchema new_col = ColumnSchemaFromPB(new_col_pb);
-        if (!new_col.is_nullable() && !new_col.has_read_default()) {
+        boost::optional<ColumnSchema> new_col;
+        RETURN_NOT_OK(ColumnSchemaFromPB(new_col_pb, &new_col));
+        if (!new_col->is_nullable() && !new_col->has_read_default()) {
           return Status::InvalidArgument(
-              Substitute("column `$0`: NOT NULL columns must have a default", new_col.name()));
+              Substitute("column `$0`: NOT NULL columns must have a default", new_col->name()));
         }
 
-        RETURN_NOT_OK(builder.AddColumn(new_col, false));
+        RETURN_NOT_OK(builder.AddColumn(*new_col, false));
         break;
       }
 
diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc
index c752dcd..1ad8dce 100644
--- a/src/kudu/tserver/tablet_service.cc
+++ b/src/kudu/tserver/tablet_service.cc
@@ -436,19 +436,25 @@ static bool GetScanPrivilegesOrRespond(const NewScanRequestPB& scan_pb,
const Sc
   unordered_set<ColumnId> required_privileges;
   // Determine the scan's projected key column IDs.
   for (int i = 0; i < scan_pb.projected_columns_size(); i++) {
-    const auto& projected_column = ColumnSchemaFromPB(scan_pb.projected_columns(i));
+    boost::optional<ColumnSchema> projected_column;
+    Status s = ColumnSchemaFromPB(scan_pb.projected_columns(i), &projected_column);
+    if (PREDICT_FALSE(!s.ok())) {
+      LOG(WARNING) << s.ToString();
+      context->RespondRpcFailure(rpc::ErrorStatusPB::ERROR_INVALID_REQUEST, s);
+      return false;
+    }
     // A projection may contain virtual columns, which don't exist in the
     // tablet schema. If we were to search for a virtual column, we would
     // incorrectly get a "not found" error. To reconcile this with the fact
     // that we want to return an authorization error if the user has requested
     // a non-virtual column that doesn't exist, we require full scan privileges
     // for virtual columns.
-    if (projected_column.type_info()->is_virtual()) {
+    if (projected_column->type_info()->is_virtual()) {
       *required_column_privileges = unordered_set<ColumnId>(schema.column_ids().begin(),
                                                             schema.column_ids().end());
       return true;
     }
-    int col_idx = schema.find_column(projected_column.name());
+    int col_idx = schema.find_column(projected_column->name());
     if (col_idx == Schema::kColumnNotFound) {
       respond_not_authorized(scan_pb.projected_columns(i).name());
       return false;
@@ -2137,31 +2143,32 @@ static Status SetupScanSpec(const NewScanRequestPB& scan_pb,
         string("Invalid predicate ") + SecureShortDebugString(pred_pb) +
         ": has no lower or upper bound.");
     }
-    ColumnSchema col(ColumnSchemaFromPB(pred_pb.column()));
-    if (projection.find_column(col.name()) == Schema::kColumnNotFound &&
-        !ContainsKey(missing_col_names, col.name())) {
-      missing_cols->push_back(col);
-      InsertOrDie(&missing_col_names, col.name());
+    boost::optional<ColumnSchema> col;
+    RETURN_NOT_OK(ColumnSchemaFromPB(pred_pb.column(), &col));
+    if (projection.find_column(col->name()) == Schema::kColumnNotFound &&
+        !ContainsKey(missing_col_names, col->name())) {
+      missing_cols->push_back(*col);
+      InsertOrDie(&missing_col_names, col->name());
     }
 
     const void* lower_bound = nullptr;
     const void* upper_bound = nullptr;
     if (pred_pb.has_lower_bound()) {
       const void* val;
-      RETURN_NOT_OK(ExtractPredicateValue(col, pred_pb.lower_bound(),
+      RETURN_NOT_OK(ExtractPredicateValue(*col, pred_pb.lower_bound(),
                                           scanner->arena(),
                                           &val));
       lower_bound = val;
     }
     if (pred_pb.has_inclusive_upper_bound()) {
       const void* val;
-      RETURN_NOT_OK(ExtractPredicateValue(col, pred_pb.inclusive_upper_bound(),
+      RETURN_NOT_OK(ExtractPredicateValue(*col, pred_pb.inclusive_upper_bound(),
                                           scanner->arena(),
                                           &val));
       upper_bound = val;
     }
 
-    auto pred = ColumnPredicate::InclusiveRange(col, lower_bound, upper_bound, scanner->arena());
+    auto pred = ColumnPredicate::InclusiveRange(*col, lower_bound, upper_bound, scanner->arena());
     if (pred) {
       VLOG(3) << Substitute("Parsed predicate $0 from $1",
                             pred->ToString(), SecureShortDebugString(scan_pb));


Mime
View raw message