msoucy / dproto

D Protocol Buffer mixins to create structures at compile time
Boost Software License 1.0
37 stars 16 forks source link

Using [packed=true] option with repeated fields causes range violations #77

Closed WebDrake closed 8 years ago

WebDrake commented 8 years ago

I've encountered an unpleasant issue with the [packed=true] option for repeated protobuf fields, as described here: https://developers.google.com/protocol-buffers/docs/proto#specifying-field-rules

The following minimal example should illustrate the issue:

import dproto.dproto;

mixin ProtocolBufferFromString!`
    message Foo {
        repeated uint32 arr = 1 [packed=true];
    }
`;

void main()
{
    Foo foo;
    foo.arr = [1u, 2, 3, 4, 5];

    auto serialized_foo = foo.serialize();

    auto foo2 = Foo(serialized_foo);  // FAILS
}

When I run this, I get an error:

core.exception.RangeError@/usr/include/d/std/bitmanip.d(2964): Range violation

Removing the [packed=true] option from the arr field allows the above program to run without error.

In a more complex protobuf setup, I have encountered a different error that seems to result from the same issue:

core.exception.AssertError@/usr/include/d/std/range/primitives.d(2200): Attempting to fetch the front of an empty array of ubyte

but I have been unable for now to reduce this to a friendly minimal example (I will keep trying to do so, and report back here if I succeed). It seems to be triggered from here: https://github.com/msoucy/dproto/blob/4e47edbdd832dd425c9139d3b335a4f0d0a10e50/import/dproto/serialize.d#L425

In either case, the deserialization problem vanishes if [packed=true] is not applied to repeated fields.

For reference, I'm using dproto 2.0.1 on Ubuntu 16.04, compiling with ldc 0.17.1.

msoucy commented 8 years ago

Yikes, that's pretty severe. I've started investigating.

WebDrake commented 8 years ago

Great, thanks! I will try to do some delving myself if I can (bit of a busy time right now with other things). Feel free to ask me to try stuff out if that would be helpful.

msoucy commented 8 years ago

I've got the issue-77 branch open, with something that seems to work for the test case.

TODO: Make it handle things that were packed, even if it isn't packed in that proto version (should be easy, just has to check for the wireType instead of isPacked when deserializing)