C++ bindings: A struct's Deserialize() now does validation before deserializing.

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

Committed: https://chromium.googlesource.com/external/mojo/+/bbe5f8b0feb702d18fda31aa86c9da59a4485a36

[vardhan, 2016-03-14 22:07:30]

PTAL

[viettrungluu, 2016-03-14 22:56:24]

https://codereview.chromium.org/1800753005/diff/1/mojo/public/cpp/bindings/te...
File mojo/public/cpp/bindings/tests/union_unittest.cc (right):

https://codereview.chromium.org/1800753005/diff/1/mojo/public/cpp/bindings/te...
mojo/public/cpp/bindings/tests/union_unittest.cc:660:
small_struct->Serialize(buf, sizeof(buf));
Does Serialize() tell you how much space it actually used (instead of forcing
you to deal with the separately via GetSerializedSize())? Should it?

https://codereview.chromium.org/1800753005/diff/1/mojo/public/cpp/bindings/te...
mojo/public/cpp/bindings/tests/union_unittest.cc:664:
EXPECT_TRUE(deserialized_struct->Deserialize(buf, sizeof(buf)));
... because presumably the actual serialized size should be what's passed in
here, no?

https://codereview.chromium.org/1800753005/diff/1/mojo/public/tools/bindings/...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
(right):

https://codereview.chromium.org/1800753005/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:32:
bool {{struct.name}}::Deserialize(void* buf, size_t buf_size) {
I do wonder whether you shouldn't expose three things:
* Validate
* Deserialize
* ValidateAndDeserialize

[vardhan, 2016-03-14 23:11:34]

https://codereview.chromium.org/1800753005/diff/1/mojo/public/cpp/bindings/te...
File mojo/public/cpp/bindings/tests/union_unittest.cc (right):

https://codereview.chromium.org/1800753005/diff/1/mojo/public/cpp/bindings/te...
mojo/public/cpp/bindings/tests/union_unittest.cc:660:
small_struct->Serialize(buf, sizeof(buf));
On 2016/03/14 22:56:23, viettrungluu wrote:
> Does Serialize() tell you how much space it actually used (instead of forcing
> you to deal with the separately via GetSerializedSize())? Should it?

There's an implied contract that Serialize() will take exactly as much as
GetSerializeSize() if successful.

I think this should be easy enough to figure out how much was consumed on
failure (by exposing the underlying FixedBuffer's counter). Not sure how the API
should change to accodomate though.. should Serialize() return it, vs. a
true/false instead? in which case, the user will be forced to do a
GetSerializedSize() just to check a success condition.

https://codereview.chromium.org/1800753005/diff/1/mojo/public/cpp/bindings/te...
mojo/public/cpp/bindings/tests/union_unittest.cc:664:
EXPECT_TRUE(deserialized_struct->Deserialize(buf, sizeof(buf)));
On 2016/03/14 22:56:23, viettrungluu wrote:
> ... because presumably the actual serialized size should be what's passed in
> here, no?

(see comment above -- GetSerializedSize() indicates how much would be written).

https://codereview.chromium.org/1800753005/diff/1/mojo/public/tools/bindings/...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
(right):

https://codereview.chromium.org/1800753005/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:32:
bool {{struct.name}}::Deserialize(void* buf, size_t buf_size) {
On 2016/03/14 22:56:23, viettrungluu wrote:
> I do wonder whether you shouldn't expose three things:
> * Validate
> * Deserialize
> * ValidateAndDeserialize

Initially, I was thinking about passing in a DeserializationOptions struct to
Deserialize() (which has a default, so you don't need to supply it most of the
time probably), which has a validation flag..

.. But I guess I can't think of a scenario where you /wouldn't/ want to validate
before deserializing? I'm not sure, but I'm okay with adding the non-validating
version.  In which case, what do you think about passing in  a  
DeserializationOptions struct?

[viettrungluu, 2016-03-14 23:37:31]

On 2016/03/14 23:11:34, vardhan wrote:
>
https://codereview.chromium.org/1800753005/diff/1/mojo/public/cpp/bindings/te...
> File mojo/public/cpp/bindings/tests/union_unittest.cc (right):
> 
>
https://codereview.chromium.org/1800753005/diff/1/mojo/public/cpp/bindings/te...
> mojo/public/cpp/bindings/tests/union_unittest.cc:660:
> small_struct->Serialize(buf, sizeof(buf));
> On 2016/03/14 22:56:23, viettrungluu wrote:
> > Does Serialize() tell you how much space it actually used (instead of
forcing
> > you to deal with the separately via GetSerializedSize())? Should it?
> 
> There's an implied contract that Serialize() will take exactly as much as
> GetSerializeSize() if successful.
> 
> I think this should be easy enough to figure out how much was consumed on
> failure (by exposing the underlying FixedBuffer's counter). Not sure how the
API
> should change to accodomate though.. should Serialize() return it, vs. a
> true/false instead? in which case, the user will be forced to do a
> GetSerializedSize() just to check a success condition.

You could, e.g., have an in-out buf_size (size_t*) parameter.

Whether it tells you the amount of space required on failure isn't
super-interesting, but for consistency it probably should do it.

The use-case would be something like:

char stack_buf[1000];
std::unique_ptr<char[]> heap_buf;
char* buf = nullptr;
size_t num_bytes = sizeof(stack_buf);
if (foo.Serialize(stack_buf, &num_bytes)) {
  // Fast path.
  buf = stack_buf;
} else {
  // Slow path.
  CHECK(num_bytes > sizeof(stack_buf)) << "D'oh";
  heap_buf.reset(new char[num_bytes]));
  // Or maybe Serialize() should accept a null buf_size param, if you don't
care?
  CHECK(foo.Serialize(heap_buf.get(), &num_bytes));
  buf = heap_buf.get();
}
// Write/send/whatever num_bytes from buf.

> 
>
https://codereview.chromium.org/1800753005/diff/1/mojo/public/cpp/bindings/te...
> mojo/public/cpp/bindings/tests/union_unittest.cc:664:
> EXPECT_TRUE(deserialized_struct->Deserialize(buf, sizeof(buf)));
> On 2016/03/14 22:56:23, viettrungluu wrote:
> > ... because presumably the actual serialized size should be what's passed in
> > here, no?
> 
> (see comment above -- GetSerializedSize() indicates how much would be
written).
> 
>
https://codereview.chromium.org/1800753005/diff/1/mojo/public/tools/bindings/...
> File
>
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
> (right):
> 
>
https://codereview.chromium.org/1800753005/diff/1/mojo/public/tools/bindings/...
>
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:32:
> bool {{struct.name}}::Deserialize(void* buf, size_t buf_size) {
> On 2016/03/14 22:56:23, viettrungluu wrote:
> > I do wonder whether you shouldn't expose three things:
> > * Validate
> > * Deserialize
> > * ValidateAndDeserialize
> 
> Initially, I was thinking about passing in a DeserializationOptions struct to
> Deserialize() (which has a default, so you don't need to supply it most of the
> time probably), which has a validation flag..
> 
> .. But I guess I can't think of a scenario where you /wouldn't/ want to
validate
> before deserializing? I'm not sure, but I'm okay with adding the
non-validating
> version.

If you trust the data source, why would you want the overhead of validating?

> In which case, what do you think about passing in  a  
> DeserializationOptions struct?

In general, I think it'd be preferable to have orthogonal functions, because it
helps with being able to layer.

E.g., layer X may take buffers assuming that they're valid (validated or
otherwise known to be valid), and deserialize from that.

Layer Y1, which gets data from an untrusted source, may wish to validate data,
before handing it to layer X. Layer Y2, which gets data from a trusted source,
may wish to just immediately hand buffers to layer X.

[qsr, 2016-03-16 08:36:53]

Could you update services/url_response_disk_cache/url_response_disk_cache_db.cc
to use this?

[vardhan, 2016-03-16 19:42:15]

(Oops, i replied to the email instead of on the codereview tool, so duplicating
it here.)

> You could, e.g., have an in-out buf_size (size_t*) parameter.
> 
> Whether it tells you the amount of space required on failure isn't
> super-interesting, but for consistency it probably should do it.
> 
> The use-case would be something like:
> 
> char stack_buf[1000];
> std::unique_ptr<char[]> heap_buf;
> char* buf = nullptr;
> size_t num_bytes = sizeof(stack_buf);
> if (foo.Serialize(stack_buf, &num_bytes)) {
>   // Fast path.
>   buf = stack_buf;
> } else {
>   // Slow path.
>   CHECK(num_bytes > sizeof(stack_buf)) << "D'oh";
>   heap_buf.reset(new char[num_bytes]));
>   // Or maybe Serialize() should accept a null buf_size param, if you don't
> care?
It can't be null if |buf_size| is also used to represent the size of the buffer.

>   CHECK(foo.Serialize(heap_buf.get(), &num_bytes));
>   buf = heap_buf.get();
> }
> // Write/send/whatever num_bytes from buf.

I guess having |buf_size| represent both "incoming buf size" and "size written"
would work..
It doesn't feel as eloquent though, although it uses a fewer # of arguments.. I
don't have a good argument against this though.

what about just separating |num_bytes|'s responsibility:
bool Serialize(void* buf, size_t buf_size, size_t* written_bytes = nullptr);

This way, you don't have to provide a |written_bytes|, and |buf_size| could be a
constant?

> If you trust the data source, why would you want the overhead of validating?
> 
> > In which case, what do you think about passing in  a  
> > DeserializationOptions struct?
> 
> In general, I think it'd be preferable to have orthogonal functions, because
it
> helps with being able to layer.
> 
> E.g., layer X may take buffers assuming that they're valid (validated or
> otherwise known to be valid), and deserialize from that.
> 
> Layer Y1, which gets data from an untrusted source, may wish to validate data,
> before handing it to layer X. Layer Y2, which gets data from a trusted source,
> may wish to just immediately hand buffers to layer X.

Sure, I'm okay with Validate(), Deserialize(), ValidateAndDeserialize().

should Deserialize() validate by default instead, and we instead introduce
DeserializeWithoutValidating()?
I'm still assuming you'll want to validate majority of the time, so i'm
proposing the "Deserialize()" name serve the majority use case

[vardhan, 2016-03-16 19:43:04]

On 2016/03/16 08:36:53, qsr wrote:
> Could you update
services/url_response_disk_cache/url_response_disk_cache_db.cc
> to use this?

That's definitely the plan (along with another consumer that isn't in the mojo
repo) -- i'll do that in a following CL.

[viettrungluu, 2016-03-16 23:08:23]

On 2016/03/16 19:42:15, vardhan wrote:
> (Oops, i replied to the email instead of on the codereview tool, so
duplicating
> it here.)
> 
> > You could, e.g., have an in-out buf_size (size_t*) parameter.
> > 
> > Whether it tells you the amount of space required on failure isn't
> > super-interesting, but for consistency it probably should do it.
> > 
> > The use-case would be something like:
> > 
> > char stack_buf[1000];
> > std::unique_ptr<char[]> heap_buf;
> > char* buf = nullptr;
> > size_t num_bytes = sizeof(stack_buf);
> > if (foo.Serialize(stack_buf, &num_bytes)) {
> >   // Fast path.
> >   buf = stack_buf;
> > } else {
> >   // Slow path.
> >   CHECK(num_bytes > sizeof(stack_buf)) << "D'oh";
> >   heap_buf.reset(new char[num_bytes]));
> >   // Or maybe Serialize() should accept a null buf_size param, if you don't
> > care?
> It can't be null if |buf_size| is also used to represent the size of the
buffer.
> 
> >   CHECK(foo.Serialize(heap_buf.get(), &num_bytes));
> >   buf = heap_buf.get();
> > }
> > // Write/send/whatever num_bytes from buf.
> 
> I guess having |buf_size| represent both "incoming buf size" and "size
written"
> would work..
> It doesn't feel as eloquent though, although it uses a fewer # of arguments..
I
> don't have a good argument against this though.
> 
> what about just separating |num_bytes|'s responsibility:
> bool Serialize(void* buf, size_t buf_size, size_t* written_bytes = nullptr);
> 
> This way, you don't have to provide a |written_bytes|, and |buf_size| could be
a
> constant?

That's fine too.

> 
> > If you trust the data source, why would you want the overhead of validating?
> > 
> > > In which case, what do you think about passing in  a  
> > > DeserializationOptions struct?
> > 
> > In general, I think it'd be preferable to have orthogonal functions, because
> it
> > helps with being able to layer.
> > 
> > E.g., layer X may take buffers assuming that they're valid (validated or
> > otherwise known to be valid), and deserialize from that.
> > 
> > Layer Y1, which gets data from an untrusted source, may wish to validate
data,
> > before handing it to layer X. Layer Y2, which gets data from a trusted
source,
> > may wish to just immediately hand buffers to layer X.
> 
> Sure, I'm okay with Validate(), Deserialize(), ValidateAndDeserialize().
> 
> should Deserialize() validate by default instead, and we instead introduce
> DeserializeWithoutValidating()?
> I'm still assuming you'll want to validate majority of the time, so i'm
> proposing the "Deserialize()" name serve the majority use case

OK.

[vardhan, 2016-03-23 00:47:19]

vardhan@google.com changed reviewers:
+ qsr@chromium.org, ukode@google.com

[vardhan, 2016-03-23 00:47:20]

PTAL

cc: ukode@, qsr@ to CR their affected files.

[qsr, 2016-03-23 08:14:37]

https://codereview.chromium.org/1800753005/diff/40001/services/url_response_d...
File services/url_response_disk_cache/url_response_disk_cache_db.cc (right):

https://codereview.chromium.org/1800753005/diff/40001/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_db.cc:31: size_t size =
GetSerializedSize_(*input);
Is it expected that I still need a private method here?

[viettrungluu, 2016-03-23 17:43:53]

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/serialization_api_unittest.cc (right):

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:43: auto
deserialize_ret = out_val.Deserialize(bytes.data(), bytes.size());
You may as well say "bool" instead of "auto", especially since you rely on this
below.

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:185: // Test
DeserializeWithoutValidation
nit: This comment would need some punctuation, but it's basically a useless
comment so you may as well just delete it.

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:187: void*
buf[100];
void*?

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:190: // Since there
is no bounds checker, this will pass even though it's bad.
Since you didn't initialize the contents of buf, you don't even know that it's
bad.

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
(right):

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:1:
{%- import "struct_macros.tmpl" as struct_macros %}
(Please make sure the .tmpl files end with a newline.)

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:38:
bool {{struct.name}}::Deserialize(void* buf, size_t buf_size) {
const void*?

Also, can't/shouldn't Deserialize() (and DeserializeWithoutValidation()) tell
you how many bytes it actually "consumed"?

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:56:
bool {{struct.name}}::DeserializeWithoutValidation(void* buf,
const void*?

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:67:
return true;
Then why does it return anything at all?

[vardhan, 2016-03-23 23:28:43]

ptal

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/serialization_api_unittest.cc (right):

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:43: auto
deserialize_ret = out_val.Deserialize(bytes.data(), bytes.size());
On 2016/03/23 17:43:53, viettrungluu wrote:
> You may as well say "bool" instead of "auto", especially since you rely on
this
> below.

Done.

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:185: // Test
DeserializeWithoutValidation
On 2016/03/23 17:43:53, viettrungluu wrote:
> nit: This comment would need some punctuation, but it's basically a useless
> comment so you may as well just delete it.

Done.

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:187: void*
buf[100];
On 2016/03/23 17:43:53, viettrungluu wrote:
> void*?

Done.

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:190: // Since there
is no bounds checker, this will pass even though it's bad.
On 2016/03/23 17:43:53, viettrungluu wrote:
> Since you didn't initialize the contents of buf, you don't even know that it's
> bad.

oops!

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
(right):

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:1:
{%- import "struct_macros.tmpl" as struct_macros %}
On 2016/03/23 17:43:53, viettrungluu wrote:
> (Please make sure the .tmpl files end with a newline.)

Done. (it wasn't -- thanks!)

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:38:
bool {{struct.name}}::Deserialize(void* buf, size_t buf_size) {
On 2016/03/23 17:43:53, viettrungluu wrote:
> const void*?
> 
> Also, can't/shouldn't Deserialize() (and DeserializeWithoutValidation()) tell
> you how many bytes it actually "consumed"?

discussed offline -- "bytes consumed" is hard to compute with pointers.

i think its possible if we enforce depth-first order when serializing a struct's
fields into a buffer: this way when deserializing, we could maintain the most
recent pointer (+ size of the object) we jump to since that would be the last
thing in the buffer.

either way, it would still require changes to the internal Deserialize_() which
involves changing some consumers.  so if this proves necessary, we could do it
in a later change.

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:56:
bool {{struct.name}}::DeserializeWithoutValidation(void* buf,
On 2016/03/23 17:43:53, viettrungluu wrote:
> const void*?

Can't right now because Deserialization is currently mutable -- we rewrite
offsets to pointers, and resetting the encoded handle offsets.

I'll add a TODO here, but making it const involves getting rid of
DecodePointersAndHandles, which is on my list of things to do for the new C++
bindings generator.

https://codereview.chromium.org/1800753005/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:67:
return true;
On 2016/03/23 17:43:53, viettrungluu wrote:
> Then why does it return anything at all?

Done.

https://codereview.chromium.org/1800753005/diff/40001/services/url_response_d...
File services/url_response_disk_cache/url_response_disk_cache_db.cc (right):

https://codereview.chromium.org/1800753005/diff/40001/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_db.cc:31: size_t size =
GetSerializedSize_(*input);
On 2016/03/23 08:14:37, qsr wrote:
> Is it expected that I still need a private method here?

No -- I forgot to update GetSerializedSize. Thanks!

[qsr, 2016-03-24 08:35:05]

url_response_disk_cache_db.cc LGTM.

[ukode, 2016-03-25 21:13:36]

lgtm

For accountdb_manager.cc, lgtm.

[vardhan, 2016-03-29 20:28:51]

ping viettrungluu@

[viettrungluu, 2016-03-30 17:49:02]

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bindings_internal.h (right):

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_internal.h:101: // TODO(vardhan): Should
RemoveStructPtr<> be internal-only? Should be /the/ way
Hmmm. Or is specializing std::pointer_traits the right thing to do? (I *think*
this is what the standard library does for, unique_ptr/shared_ptr/etc.)

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/serialization_api_unittest.cc (right):

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:104: char
buf[1000];
Please initialize this (|= {}| is sufficient).

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:107: size_t
bytes_written;
Please initialize this, preferably to something that isn't 160.

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:168: char*
buf[100];
a) Why is this an array of pointers?
b) Please initialize this.

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:180:
EmptyStruct::Data_* es_data = reinterpret_cast<EmptyStruct::Data_*>(buf);
buf being an array of char*'s makes this UB.

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:187: char*
buf[100];
"

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:193: memset(buf, 0,
sizeof(buf));
And by initializing, you can skip this.

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
(right):

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:42:
std::string err_str;
You probably shouldn't unconditionally have an |std::string| for non-debug
builds.

[vardhan, 2016-03-30 22:49:07]

ptal

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bindings_internal.h (right):

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_internal.h:101: // TODO(vardhan): Should
RemoveStructPtr<> be internal-only? Should be /the/ way
On 2016/03/30 17:49:01, viettrungluu wrote:
> Hmmm. Or is specializing std::pointer_traits the right thing to do? (I *think*
> this is what the standard library does for, unique_ptr/shared_ptr/etc.)

yeah, good point;  std::pointer_traits will definitely serve both
RemoveStructPtr<> (via pointer_traits::element_type) and UnwrapStructPtr <> (via
pointer_traits::pointer).

i'll change the TODO to reflect this.

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/serialization_api_unittest.cc (right):

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:104: char
buf[1000];
On 2016/03/30 17:49:01, viettrungluu wrote:
> Please initialize this (|= {}| is sufficient).

Done.

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:107: size_t
bytes_written;
On 2016/03/30 17:49:01, viettrungluu wrote:
> Please initialize this, preferably to something that isn't 160.

Done.

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:168: char*
buf[100];
On 2016/03/30 17:49:01, viettrungluu wrote:
> a) Why is this an array of pointers?
Oops!  thanks for catching this, bad typo.

> b) Please initialize this.
Done.

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:180:
EmptyStruct::Data_* es_data = reinterpret_cast<EmptyStruct::Data_*>(buf);
On 2016/03/30 17:49:01, viettrungluu wrote:
> buf being an array of char*'s makes this UB.

Acknowledged.

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:187: char*
buf[100];
On 2016/03/30 17:49:01, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_api_unittest.cc:193: memset(buf, 0,
sizeof(buf));
On 2016/03/30 17:49:01, viettrungluu wrote:
> And by initializing, you can skip this.

Done.

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
(right):

https://codereview.chromium.org/1800753005/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:42:
std::string err_str;
On 2016/03/30 17:49:01, viettrungluu wrote:
> You probably shouldn't unconditionally have an |std::string| for non-debug
> builds.

Done.

[viettrungluu, 2016-03-31 17:36:55]

lgtm

[vardhan, 2016-03-31 20:03:41]

Description was changed from

==========
C++ bindings: A struct's Deserialize() now does validation before deserializing.
* Consequently, |Deserialize()| now has a |buf_size| argument.

BUG=#419
R=viettrungluu@chromium.org
==========

to

==========
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

Committed:
https://chromium.googlesource.com/external/mojo/+/bbe5f8b0feb702d18fda31aa86c...
==========

[vardhan, 2016-03-31 20:03:43]

Committed patchset #5 (id:80001) manually as
bbe5f8b0feb702d18fda31aa86c9da59a4485a36 (presubmit successful).