C bindings: Implement _Validate(), and some pre-requisites

C bindings: Implement _Validate(), and some pre-requisites

This patchset passes the conformance & bounds checking validation tests,
the same ones that the C++ version goes through.

Pre-req to accomodate getting validation to work:
- For structs, code-generate version->expected struct size mapping, used
  for validation.
- For arrays, code-generate size of a single element, in bits. Used for
  array size validation.
- For maps, have a separate type descriptor type, just so we can check
  that the 2 contained arrays are the same size.

Validation involves maintaining a ValidationContext struct, which
throughout validation is updated to containing the expected next handle
offset and next pointer offset.

This CL also factors out some common validation data/golden file reading code between C/C++.

R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/15fbc30f08a51f9ead881822c9572fbe013aecba

[vardhan, 2016-07-19 20:28:00]

PTAL

Sorry about the large code size -- It involves some small things I had to do --
as I found out about them while getting the validation unittests to work -- and
were driven by getting _Validate() to work.  If it's too confusing, please let
me know and I can try to split it up.

[vardhan, 2016-07-26 20:12:53]

Description was changed from

==========
C bindings: Implement _Validate(), and some pre-requisites

This patchset passes the conformance & bounds checking validation tests,
the same ones that the C++ version goes through.

Pre-req to accomodate getting validation to work:
- For structs, code-generate version->expected struct size mapping, used
  for validation.
- For arrays, code-generate size of a single element, in bits. Used for
  array size validation.
- For maps, have a separate type descriptor type, just so we can check
  that the 2 contained arrays are the same size.
- Fix the type descriptors generated for maps: the key/value types
  weren't wrapped in an array.

Validation involves maintaining a ValidationContext struct, which
throughout validation is updated to containing the expected next handle
offset and next pointer offset.

R=viettrungluu@chromium.org
==========

to

==========
C bindings: Implement _Validate(), and some pre-requisites

This patchset passes the conformance & bounds checking validation tests,
the same ones that the C++ version goes through.

Pre-req to accomodate getting validation to work:
- For structs, code-generate version->expected struct size mapping, used
  for validation.
- For arrays, code-generate size of a single element, in bits. Used for
  array size validation.
- For maps, have a separate type descriptor type, just so we can check
  that the 2 contained arrays are the same size.

Validation involves maintaining a ValidationContext struct, which
throughout validation is updated to containing the expected next handle
offset and next pointer offset.

This CL also factors out some common validation data/golden file reading code
between C/C++.

R=viettrungluu@chromium.org
==========

[vardhan, 2016-07-26 20:13:40]

ping.

(i moved out some of generator changes into a separate CL to simplify this one)

[viettrungluu, 2016-07-28 23:03:01]

Just minor things.

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/message.c (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:16: if (in_buf_size < sizeof(struct
MojomStructHeader) ||
For arrays, you return "illegal memory range" for the first check, but
"unexpected ... header" for the second. Should you do the same here? (Or should
you do there what you do here?)

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:62: return !(msg->flags &
MOJOM_MESSAGE_FLAGS_EXPECTS_RESPONSE) &&
nit: I'd suggest always putting (...) around the entire condition when you're
using ?: (unless the condition is just one thing (like a single function call or
a single variable name).

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:71: return msg->flags &
MOJOM_MESSAGE_FLAGS_IS_RESPONSE
"

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/type_descriptor.c (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/type_descriptor.c:286: if (pointer->offset >=
UINT32_MAX || pointer->offset > max_offset)
Not that it really makes a difference, but why is the (first) comparison a >=
instead of >? (According to your comment, it should be >.)

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/type_descriptor.h (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/type_descriptor.h:144: uint8_t elem_num_bits;
I tend to think that using a uint8_t here is probably a premature optimization
(also note that reading uint8_t's may be more expensive on some architectures).

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/BUILD.gn:99:
mojo_sdk_source_set("validation_util") {
Do you really need a separate target here, instead of moving it into the
mojo_public_bindings_test_utils target above?

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/validation_util.h (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/validation_util.h:34: }  // namespace mojo
The order of these "// namespace ..." comments is backwards.

https://codereview.chromium.org/2163793002/diff/60001/mojom/generators/c/c_ge...
File mojom/generators/c/c_generator.go (left):

https://codereview.chromium.org/2163793002/diff/60001/mojom/generators/c/c_ge...
mojom/generators/c/c_generator.go:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
You didn't mean to delete this line.

https://codereview.chromium.org/2163793002/diff/60001/mojom/generators/c/cgen...
File mojom/generators/c/cgen/type_table.go (right):

https://codereview.chromium.org/2163793002/diff/60001/mojom/generators/c/cgen...
mojom/generators/c/cgen/type_table.go:128: 
Did you really mean to add a blank line here?

[vardhan, 2016-07-29 21:29:22]

PTAL

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/message.c (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:16: if (in_buf_size < sizeof(struct
MojomStructHeader) ||
On 2016/07/28 23:03:01, viettrungluu wrote:
> For arrays, you return "illegal memory range" for the first check, but
> "unexpected ... header" for the second. Should you do the same here? (Or
should
> you do there what you do here?)
True -- this should be split up (but i guess the validation tests don't exercise
this enough)

neither :( there is an inconsistency here -- struct and message headers are this
way (since message headers are structs), but arrays are for some reason
different.  we should fix this later, I'll open up a bug for it

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:62: return !(msg->flags &
MOJOM_MESSAGE_FLAGS_EXPECTS_RESPONSE) &&
On 2016/07/28 23:03:01, viettrungluu wrote:
> nit: I'd suggest always putting (...) around the entire condition when you're
> using ?: (unless the condition is just one thing (like a single function call
or
> a single variable name).

Done.

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/message.c:71: return msg->flags &
MOJOM_MESSAGE_FLAGS_IS_RESPONSE
On 2016/07/28 23:03:01, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/type_descriptor.c (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/type_descriptor.c:286: if (pointer->offset >=
UINT32_MAX || pointer->offset > max_offset)
On 2016/07/28 23:03:01, viettrungluu wrote:
> Not that it really makes a difference, but why is the (first) comparison a >=
> instead of >? (According to your comment, it should be >.)

the comment says <2^32, not <=. so checking that it's not >= seems fine to me?

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/type_descriptor.h (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/type_descriptor.h:144: uint8_t elem_num_bits;
On 2016/07/28 23:03:01, viettrungluu wrote:
> I tend to think that using a uint8_t here is probably a premature optimization
> (also note that reading uint8_t's may be more expensive on some
architectures).

I'll change it to uint32_t.

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/BUILD.gn:99:
mojo_sdk_source_set("validation_util") {
On 2016/07/28 23:03:01, viettrungluu wrote:
> Do you really need a separate target here, instead of moving it into the
> mojo_public_bindings_test_utils target above?

yeah, i tried this initially :( mojo_public_bindings_test_utils is used by other
languages (cython) to parse validation test+golden files. And since this target
has some gtest references, it would force mojo_public_bindings_test_utils to be
testonly, which created some problems for cython.  It was much cleaner to just
do this (or remove gtest dependencies, which means we lose some ADD_FAILURES()
which i think we should keep)

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/validation_util.h (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/validation_util.h:34: }  // namespace mojo
On 2016/07/28 23:03:01, viettrungluu wrote:
> The order of these "// namespace ..." comments is backwards.

oops, thanks!

https://codereview.chromium.org/2163793002/diff/60001/mojom/generators/c/c_ge...
File mojom/generators/c/c_generator.go (left):

https://codereview.chromium.org/2163793002/diff/60001/mojom/generators/c/c_ge...
mojom/generators/c/c_generator.go:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
On 2016/07/28 23:03:01, viettrungluu wrote:
> You didn't mean to delete this line.

I don't know why, but this keeps happening (and usually i go back and fix it).

https://codereview.chromium.org/2163793002/diff/60001/mojom/generators/c/cgen...
File mojom/generators/c/cgen/type_table.go (right):

https://codereview.chromium.org/2163793002/diff/60001/mojom/generators/c/cgen...
mojom/generators/c/cgen/type_table.go:128: 
On 2016/07/28 23:03:01, viettrungluu wrote:
> Did you really mean to add a blank line here?

Done.

[viettrungluu, 2016-08-01 21:17:19]

lgtm otherwise

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/type_descriptor.c (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/type_descriptor.c:286: if (pointer->offset >=
UINT32_MAX || pointer->offset > max_offset)
On 2016/07/29 21:29:21, vardhan wrote:
> On 2016/07/28 23:03:01, viettrungluu wrote:
> > Not that it really makes a difference, but why is the (first) comparison a
>=
> > instead of >? (According to your comment, it should be >.)
> 
> the comment says <2^32, not <=. so checking that it's not >= seems fine to me?

UINT32_MAX is the maximum value of a uint32_t, which is 2^32 - 1.

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/BUILD.gn:99:
mojo_sdk_source_set("validation_util") {
On 2016/07/29 21:29:22, vardhan wrote:
> On 2016/07/28 23:03:01, viettrungluu wrote:
> > Do you really need a separate target here, instead of moving it into the
> > mojo_public_bindings_test_utils target above?
> 
> yeah, i tried this initially :( mojo_public_bindings_test_utils is used by
other
> languages (cython) to parse validation test+golden files. And since this
target
> has some gtest references, it would force mojo_public_bindings_test_utils to
be
> testonly, which created some problems for cython.  It was much cleaner to just
> do this (or remove gtest dependencies, which means we lose some ADD_FAILURES()
> which i think we should keep)

Blech. Not having a gtest dependency seems reasonable, but it seems to me that
whatever cython target is depending on mojo_public_bindings_test_utils should be
testonly (and so should mojo_public_bindings_test_utils). Could you add a TODO
about this?

[vardhan, 2016-08-01 22:18:17]

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
File mojo/public/c/bindings/lib/type_descriptor.c (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/c/bindings/...
mojo/public/c/bindings/lib/type_descriptor.c:286: if (pointer->offset >=
UINT32_MAX || pointer->offset > max_offset)
On 2016/08/01 21:17:19, viettrungluu wrote:
> On 2016/07/29 21:29:21, vardhan wrote:
> > On 2016/07/28 23:03:01, viettrungluu wrote:
> > > Not that it really makes a difference, but why is the (first) comparison a
> >=
> > > instead of >? (According to your comment, it should be >.)
> > 
> > the comment says <2^32, not <=. so checking that it's not >= seems fine to
me?
> 
> UINT32_MAX is the maximum value of a uint32_t, which is 2^32 - 1.

oh right! both the condition and the comment are true to be honest haha. ill
resolve by making the comment be <= UINT32_MAX, and checking the offset isn't >
UINT32_MAX

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/2163793002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/BUILD.gn:99:
mojo_sdk_source_set("validation_util") {
On 2016/08/01 21:17:19, viettrungluu wrote:
> On 2016/07/29 21:29:22, vardhan wrote:
> > On 2016/07/28 23:03:01, viettrungluu wrote:
> > > Do you really need a separate target here, instead of moving it into the
> > > mojo_public_bindings_test_utils target above?
> > 
> > yeah, i tried this initially :( mojo_public_bindings_test_utils is used by
> other
> > languages (cython) to parse validation test+golden files. And since this
> target
> > has some gtest references, it would force mojo_public_bindings_test_utils to
> be
> > testonly, which created some problems for cython.  It was much cleaner to
just
> > do this (or remove gtest dependencies, which means we lose some
ADD_FAILURES()
> > which i think we should keep)
> 
> Blech. Not having a gtest dependency seems reasonable, but it seems to me that
> whatever cython target is depending on mojo_public_bindings_test_utils should
be
> testonly (and so should mojo_public_bindings_test_utils). Could you add a TODO
> about this?

Done.

[vardhan, 2016-08-01 22:18:42]

Description was changed from

==========
C bindings: Implement _Validate(), and some pre-requisites

This patchset passes the conformance & bounds checking validation tests,
the same ones that the C++ version goes through.

Pre-req to accomodate getting validation to work:
- For structs, code-generate version->expected struct size mapping, used
  for validation.
- For arrays, code-generate size of a single element, in bits. Used for
  array size validation.
- For maps, have a separate type descriptor type, just so we can check
  that the 2 contained arrays are the same size.

Validation involves maintaining a ValidationContext struct, which
throughout validation is updated to containing the expected next handle
offset and next pointer offset.

This CL also factors out some common validation data/golden file reading code
between C/C++.

R=viettrungluu@chromium.org
==========

to

==========
C bindings: Implement _Validate(), and some pre-requisites

This patchset passes the conformance & bounds checking validation tests,
the same ones that the C++ version goes through.

Pre-req to accomodate getting validation to work:
- For structs, code-generate version->expected struct size mapping, used
  for validation.
- For arrays, code-generate size of a single element, in bits. Used for
  array size validation.
- For maps, have a separate type descriptor type, just so we can check
  that the 2 contained arrays are the same size.

Validation involves maintaining a ValidationContext struct, which
throughout validation is updated to containing the expected next handle
offset and next pointer offset.

This CL also factors out some common validation data/golden file reading code
between C/C++.

R=viettrungluu@chromium.org

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

[vardhan, 2016-08-01 22:18:44]

Committed patchset #6 (id:100001) manually as
15fbc30f08a51f9ead881822c9572fbe013aecba (presubmit successful).