C++ bindings: A struct's Deserialize() now does validation before deserializing.
* Consequently, |Deserialize()| now has a |buf_size| argument.
BUG=#419
R=qsr@chromium.org, ukode@google.com, viettrungluu@chromium.org
Review URL: https://codereview.chromium.org/1800753005 .
diff --git a/examples/serialization/main.cc b/examples/serialization/main.cc
index 9b0a96d..381ab97 100644
--- a/examples/serialization/main.cc
+++ b/examples/serialization/main.cc
@@ -19,8 +19,7 @@
char buf[1000];
MOJO_CHECK(in.Serialize(buf, sizeof(buf)));
-
- out.Deserialize(buf);
+ MOJO_CHECK(out.Deserialize(buf, sizeof(buf)));
MOJO_CHECK(out.a == 1);
MOJO_CHECK(out.b == 2.0f);
MOJO_CHECK(out.c == "hello world!");
diff --git a/mojo/public/cpp/bindings/lib/bindings_internal.h b/mojo/public/cpp/bindings/lib/bindings_internal.h
index 7889fa6..7479cd1 100644
--- a/mojo/public/cpp/bindings/lib/bindings_internal.h
+++ b/mojo/public/cpp/bindings/lib/bindings_internal.h
@@ -98,6 +98,8 @@
enum { value = std::is_base_of<Handle, H>::value };
};
+// TODO(vardhan): Replace RemoveStructPtr<> and UnwrapStructPtr<> with
+// specializations of std::pointer_traits<> on [Inlined]StructPtr<>.
template <typename T>
struct RemoveStructPtr {
typedef T type;
diff --git a/mojo/public/cpp/bindings/lib/fixed_buffer.cc b/mojo/public/cpp/bindings/lib/fixed_buffer.cc
index c81fc6e..44158c2 100644
--- a/mojo/public/cpp/bindings/lib/fixed_buffer.cc
+++ b/mojo/public/cpp/bindings/lib/fixed_buffer.cc
@@ -17,14 +17,14 @@
FixedBuffer::FixedBuffer() : ptr_(nullptr), cursor_(0), size_(0) {}
void FixedBuffer::Initialize(void* memory, size_t size) {
- MOJO_DCHECK(size == internal::Align(size));
-
ptr_ = static_cast<char*>(memory);
cursor_ = 0;
size_ = size;
}
void* FixedBuffer::Allocate(size_t delta) {
+ // Ensure that all memory returned by Allocate() is 8-byte aligned w.r.t the
+ // start of the buffer.
delta = internal::Align(delta);
if (delta == 0 || delta > size_ - cursor_) {
diff --git a/mojo/public/cpp/bindings/lib/fixed_buffer.h b/mojo/public/cpp/bindings/lib/fixed_buffer.h
index 83eaf97..45d0411 100644
--- a/mojo/public/cpp/bindings/lib/fixed_buffer.h
+++ b/mojo/public/cpp/bindings/lib/fixed_buffer.h
@@ -44,9 +44,17 @@
size_t size() const { return size_; }
+ // Returns the number of bytes used so far.
+ // TODO(vardhan): Introduce this method in |Buffer|? Doesn't seem necessary.
+ size_t BytesUsed() const { return cursor_; }
+
// Grows the buffer by |num_bytes| and returns a pointer to the start of the
// addition. The resulting address is 8-byte aligned, and the content of the
// memory is zero-filled.
+ // TODO(vardhan): Allocate() should safely fail if we run out of buffer space.
+ // This will allow us to, e.g, fail when trying to consume a buffer to
+ // serialize into, and return an insufficient space error. Currently, there
+ // are consumers of FixedBuffer that rely on it CHECK-failing.
void* Allocate(size_t num_bytes) override;
protected:
diff --git a/mojo/public/cpp/bindings/lib/validation_errors.h b/mojo/public/cpp/bindings/lib/validation_errors.h
index d152333..da8fa3f 100644
--- a/mojo/public/cpp/bindings/lib/validation_errors.h
+++ b/mojo/public/cpp/bindings/lib/validation_errors.h
@@ -63,6 +63,7 @@
const char* ValidationErrorToString(ValidationError error);
+// TODO(vardhan): This can die, along with |ValidationErrorObserverForTesting|.
void ReportValidationError(ValidationError error,
std::string* description = nullptr);
diff --git a/mojo/public/cpp/bindings/tests/buffer_unittest.cc b/mojo/public/cpp/bindings/tests/buffer_unittest.cc
index 317a2a5..96acdd9 100644
--- a/mojo/public/cpp/bindings/tests/buffer_unittest.cc
+++ b/mojo/public/cpp/bindings/tests/buffer_unittest.cc
@@ -21,6 +21,14 @@
return true;
}
+// Tests that you can create a FixedBuffer whose underlying buffer size is not
+// a multiple of 8.
+TEST(FixedBufferTest, UnAlignedBufferSized) {
+ char char_buf[10] = {};
+ internal::FixedBuffer fixed_buf;
+ fixed_buf.Initialize(char_buf, sizeof(char_buf));
+}
+
// Tests that FixedBuffer allocates memory aligned to 8 byte boundaries.
TEST(FixedBufferTest, Alignment) {
internal::FixedBufferForTesting buf(internal::Align(10) * 2);
diff --git a/mojo/public/cpp/bindings/tests/serialization_api_unittest.cc b/mojo/public/cpp/bindings/tests/serialization_api_unittest.cc
index 28ada48..cd711dc 100644
--- a/mojo/public/cpp/bindings/tests/serialization_api_unittest.cc
+++ b/mojo/public/cpp/bindings/tests/serialization_api_unittest.cc
@@ -23,25 +23,29 @@
void SerializeAndDeserialize(
Type* val,
mojo::internal::ValidationError expected_validation_error) {
+ size_t bytes_written = 0;
size_t num_bytes = val->GetSerializedSize();
std::vector<uint8_t> bytes(num_bytes + 1);
// Last byte is a magic value, helps catch a buffer overflow for
// serialization.
bytes[num_bytes] = 170;
- val->Serialize(bytes.data(), num_bytes);
+ val->Serialize(bytes.data(), num_bytes, &bytes_written);
EXPECT_EQ(170u, bytes[num_bytes]);
+ EXPECT_EQ(num_bytes, bytes_written);
mojo::internal::BoundsChecker bounds_checker(bytes.data(), num_bytes, 0);
auto actual_validation_error =
Type::Data_::Validate(bytes.data(), &bounds_checker, nullptr);
EXPECT_EQ(expected_validation_error, actual_validation_error);
+ Type out_val;
+ bool deserialize_ret = out_val.Deserialize(bytes.data(), bytes.size());
if (actual_validation_error == mojo::internal::ValidationError::NONE) {
- Type out_val;
- out_val.Deserialize(bytes.data());
EXPECT_TRUE(val->Equals(out_val));
}
+ EXPECT_EQ(actual_validation_error == mojo::internal::ValidationError::NONE,
+ deserialize_ret);
}
private:
@@ -90,6 +94,23 @@
SerializeAndDeserialize(&default_values,
mojo::internal::ValidationError::NONE);
}
+
+ {
+ SCOPED_TRACE("NoDefaultFieldValues.Serialize() should fail");
+ NoDefaultFieldValues nd;
+ nd.f0 = true;
+ nd.f23 = mojo::Array<mojo::String>::New(10);
+
+ char buf[1000] = {};
+ EXPECT_FALSE(nd.Serialize(buf, sizeof(buf)));
+
+ size_t bytes_written = 0;
+ EXPECT_FALSE(nd.Serialize(buf, sizeof(buf), &bytes_written));
+ EXPECT_EQ(160UL, bytes_written);
+ // The Serialize() shouldn't get around to reserving space for the |f23|
+ // array field.
+ EXPECT_LT(bytes_written, nd.GetSerializedSize());
+ }
}
// This tests serialization of handles -- These should be deaths or
@@ -142,6 +163,46 @@
mojo::internal::ValidationError::NONE);
}
+// Test that |Deserialize()| appropriately fails on validation.
+TEST_F(StructSerializationAPITest, DeserializationFailure) {
+ char buf[100] = {};
+ EmptyStruct es;
+
+ // Bounds checker should fail this, since buf_size is too small.
+ EXPECT_FALSE(es.Deserialize(buf, 1));
+
+ es.Serialize(buf, sizeof(buf));
+ EXPECT_TRUE(es.Deserialize(buf, sizeof(buf)));
+
+ // Invalid struct header: this should happen inside
+ // EmptyStruct::Data_::Validate()).
+ es.Serialize(buf, sizeof(buf));
+ EmptyStruct::Data_* es_data = reinterpret_cast<EmptyStruct::Data_*>(buf);
+ es_data->header_.num_bytes = 0;
+ EXPECT_FALSE(es.Deserialize(buf, sizeof(buf)));
+}
+
+TEST_F(StructSerializationAPITest, DeserializationWithoutValidation) {
+ const int32_t kMagic = 456;
+ char buf[100] = {};
+ ContainsOther::Data_* co_data = reinterpret_cast<ContainsOther::Data_*>(buf);
+ ContainsOther co;
+
+ // Success case.
+ co.other = kMagic;
+ EXPECT_TRUE(co.Serialize(buf, sizeof(buf)));
+ co.other = 0;
+ co.DeserializeWithoutValidation(buf);
+ EXPECT_EQ(kMagic, co.other);
+
+ // Invalid struct header, but will pass (i.e., won't crash) anyway because we
+ // don't Validate.
+ co_data->header_.num_bytes = 0u;
+ co.DeserializeWithoutValidation(buf);
+ EXPECT_EQ(kMagic, co_data->other);
+ EXPECT_EQ(0u, co_data->header_.num_bytes);
+}
+
} // namespace
} // namespace test
} // namespace mojo
diff --git a/mojo/public/cpp/bindings/tests/union_unittest.cc b/mojo/public/cpp/bindings/tests/union_unittest.cc
index afdfbbf..13f3b5e 100644
--- a/mojo/public/cpp/bindings/tests/union_unittest.cc
+++ b/mojo/public/cpp/bindings/tests/union_unittest.cc
@@ -663,8 +663,7 @@
EXPECT_TRUE(small_struct->pod_union.is_null());
SmallStructPtr deserialized_struct = SmallStruct::New();
- deserialized_struct->Deserialize(buf);
-
+ EXPECT_TRUE(deserialized_struct->Deserialize(buf, sizeof(buf)));
EXPECT_TRUE(deserialized_struct->pod_union.is_null());
}
diff --git a/mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl b/mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
index 6ac6a3e..e80b405 100644
--- a/mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
+++ b/mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
@@ -5,34 +5,70 @@
}
bool {{struct.name}}::Serialize(void* buf,
- size_t buf_size) {
+ size_t buf_size,
+ size_t* bytes_written) {
+ MOJO_DCHECK(buf);
+
mojo::internal::FixedBuffer overlay_buf;
overlay_buf.Initialize(buf, buf_size);
internal::{{struct.name}}_Data* output_ptr;
- if (Serialize_(this, &overlay_buf, &output_ptr) !=
- mojo::internal::ValidationError::NONE) {
+ auto err = Serialize_(this, &overlay_buf, &output_ptr);
+ if (err != mojo::internal::ValidationError::NONE) {
// TODO(vardhan): Once Serialize_() outputs handles that it serialized
// (even partially, if there are failures), we should CHECK fail here if
// handles are non-empty.
+ MOJO_DLOG(ERROR) << "Could not serialize: " <<
+ mojo::internal::ValidationErrorToString(err);
+
+ if (bytes_written)
+ *bytes_written = overlay_buf.BytesUsed();
return false;
}
std::vector<mojo::Handle> handles;
output_ptr->EncodePointersAndHandles(&handles);
-
MOJO_CHECK(handles.empty()) << "Serialize() does not support handles.";
+ if (bytes_written)
+ *bytes_written = overlay_buf.BytesUsed();
return true;
}
-void {{struct.name}}::Deserialize(void* buf) {
+bool {{struct.name}}::Deserialize(void* buf, size_t buf_size) {
+ MOJO_DCHECK(buf);
+
+ mojo::internal::BoundsChecker checker(buf, buf_size, 0);
+
+ std::string* err_str = nullptr;
+#if !defined(NDEBUG)
+ std::string err_str2;
+ err_str = &err_str2;
+#endif
+
+ mojo::internal::ValidationError err =
+ internal::{{struct.name}}_Data::Validate(buf, &checker, err_str);
+ if (err != mojo::internal::ValidationError::NONE) {
+ MOJO_DLOG(ERROR) << "Deserialization error "
+ << mojo::internal::ValidationErrorToString(err)
+ << ": " << *err_str;
+ return false;
+ }
+
+ DeserializeWithoutValidation(buf);
+ return true;
+}
+
+// TODO(vardhan): Make this |buf| a |const void*| once deserialization becomes
+// immutable.
+void {{struct.name}}::DeserializeWithoutValidation(void* buf) {
+ MOJO_DCHECK(buf);
+
internal::{{struct.name}}_Data* input =
static_cast<internal::{{struct.name}}_Data*>(buf);
-
std::vector<mojo::Handle> handles;
input->DecodePointersAndHandles(&handles);
- MOJO_CHECK(handles.empty()) << "Deserialize() does not support handles.";
+ MOJO_CHECK(handles.empty()) << "Deserialization does not support handles.";
Deserialize_(input, this);
}
diff --git a/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl b/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
index 339f94f..1b976cf 100644
--- a/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
+++ b/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
@@ -35,28 +35,27 @@
// Returns the number of bytes it would take to serialize this struct's data.
size_t GetSerializedSize() const;
- // Returns true on successful serialization. On failure, part of the data may
+ // Returns true on successful serialization. On failure, part of the data may
// be serialized, until the point of failure. This API does not support
- // serializing handles.
+ // serializing handles. If not null, |bytes_written| is set to the number of
+ // bytes written to |buf|, even if this function return false.
//
// TODO(vardhan): For now, we return true for success. Should we define a
- // public error type for serialization? (we shouldn't reuse
- // internal::ValidationError).
- bool Serialize(void* buf, size_t buf_size);
+ // public error type for serialization? Should we open up
+ // internal::ValidationError?
+ bool Serialize(void* buf, size_t buf_size, size_t* bytes_written = nullptr);
+
+ // Deserializes the given |buf| of size |buf_size| representing a serialized
+ // version of this struct. The buffer is validated before it is deserialized.
+ // Returns true on successful deserialization.
+ // TODO(vardhan): Recover the validation error if there is one?
+ bool Deserialize(void* buf, size_t buf_size);
// Deserializes the given |buf| representing a serialized version of this
- // struct. Assumes that the serialized |buf| is valid.
- //
- // TODO(vardhan):
- // - Should we pass in how big |buf| is and validate that it is
- // <= GetSerializedSize()? If so, should this validation happen all the
- // time?
- // - Deserialize() will CHECK-fail if you try to deserialize something with
- // a bad offset, etc. For IPC, we |Validate()| before running
- // deserialization if we want safety, but we probably want a recoverable
- // error for this API.
- // - What's the validation story?
- void Deserialize(void* buf);
+ // struct. The buffer is NOT validated before it is deserialized, so the user
+ // must be confident of its validity and that |buf| points to enough data to
+ // finish deserializing.
+ void DeserializeWithoutValidation(void* buf);
{% if struct|is_cloneable_kind %}
{{struct.name}}Ptr Clone() const;
diff --git a/services/authentication/accounts_db_manager.cc b/services/authentication/accounts_db_manager.cc
index f7cd3fe..fe0a1ad 100644
--- a/services/authentication/accounts_db_manager.cc
+++ b/services/authentication/accounts_db_manager.cc
@@ -181,20 +181,11 @@
if (bytes_read.size() != 0) {
// Deserialize data from file
const char* data = reinterpret_cast<const char*>(&bytes_read[0]);
-
- // Validate the file contents before deserializing
- mojo::internal::BoundsChecker bounds_checker(data, bytes_read.size(), 0);
- std::string error;
- mojo::internal::ValidationError verror =
- internal::CredentialStore_Data::Validate(data, &bounds_checker, &error);
- if (verror != mojo::internal::ValidationError::NONE) {
- LOG(ERROR) << "Validation() error on accounts db ["
- << ValidationErrorToString(verror) << "][" << error << "]";
+ if (!creds_store_.Deserialize((void*)data, bytes_read.size())) {
+ LOG(ERROR) << "Deserialize() error on accounts db.";
error_ = CREDENTIALS_DB_VALIDATE_ERROR;
return;
}
-
- creds_store_.Deserialize((void*)data);
// When we have multiple versions, this is not a fatal error, but a sign
// that we need to update (or reinitialize) the db.
CHECK_EQ(creds_store_.version, kCredsDbVersion);
@@ -219,17 +210,12 @@
if (bytes_read.size() != 0) {
// Deserialize data from file
const char* data = reinterpret_cast<const char*>(&bytes_read[0]);
-
- // Validate the file contents before deserializing
- mojo::internal::BoundsChecker bounds_checker(data, bytes_read.size(), 0);
- if (internal::Db_Data::Validate(data, &bounds_checker, nullptr) !=
- mojo::internal::ValidationError::NONE) {
- LOG(ERROR) << "Validation() error on auth db.";
+ if (!auth_grants_.Deserialize((void*)data, bytes_read.size())) {
+ LOG(ERROR) << "Deserialize() error on auth db.";
error_ = AUTHORIZATIONS_DB_VALIDATE_ERROR;
return;
}
- auth_grants_.Deserialize((void*)data);
// When we have multiple versions, this is not a fatal error, but a sign
// that we need to update (or reinitialize) the db.
CHECK_EQ(auth_grants_.version, kAuthDbVersion);
diff --git a/services/url_response_disk_cache/url_response_disk_cache_db.cc b/services/url_response_disk_cache/url_response_disk_cache_db.cc
index fc06897..a96b34f 100644
--- a/services/url_response_disk_cache/url_response_disk_cache_db.cc
+++ b/services/url_response_disk_cache/url_response_disk_cache_db.cc
@@ -16,7 +16,6 @@
#include "base/time/time.h"
#include "leveldb/comparator.h"
#include "leveldb/db.h"
-#include "mojo/public/cpp/bindings/lib/fixed_buffer.h"
#include "services/url_response_disk_cache/url_response_disk_cache_entry.mojom.h"
namespace mojo {
@@ -26,40 +25,20 @@
// character.
const char kVersionKey[] = "\1version";
-// TODO(vardhan): Rework these to use the public Serialize/Deserialize
-// functions.
+// T is a |StructPtr|.
template <typename T>
void Serialize(T input, std::string* output) {
- typedef typename mojo::internal::WrapperTraits<T>::DataType DataType;
- size_t size = GetSerializedSize_(*input);
-
+ size_t size = input->GetSerializedSize();
output->clear();
output->resize(size);
- mojo::internal::FixedBuffer buf;
- buf.Initialize(&output->at(0), size);
-
- DataType data_type;
- Serialize_(input.get(), &buf, &data_type);
- std::vector<Handle> handles;
- data_type->EncodePointersAndHandles(&handles);
+ input->Serialize(&output->at(0), size);
}
template <typename T>
bool Deserialize(void* data, size_t size, T* output) {
- typedef typename mojo::internal::WrapperTraits<T>::DataType DataType;
- mojo::internal::BoundsChecker bounds_checker(data, size, 0);
- if (std::remove_pointer<DataType>::type::Validate(data, &bounds_checker,
- nullptr) !=
- mojo::internal::ValidationError::NONE)
- return false;
-
- DataType data_type = reinterpret_cast<DataType>(data);
- std::vector<Handle> handles;
- data_type->DecodePointersAndHandles(&handles);
*output = mojo::internal::RemoveStructPtr<T>::type::New();
- Deserialize_(data_type, output->get());
- return true;
+ return (*output)->Deserialize(data, size);
}
template <typename T>
