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 <[email protected]>
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 <[email protected]>
---
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));
