Fix encoding alignment logic. #18
Conversation
The newEncoder method is recursive, but alignment logic must be based on "global" absolute position. This FIX allows recursive calls to set the initial position. Unit tests added to very the problem and fix. The problem was discovered for nested containers where the start of the nested container can be on a 4-byte-aligned (but not 8-byte-aligned) boundary.
|
lgtm from eyeballing both this and the dbus wire format: http://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-marshaling I would absolutely love @alexlarsson to review this one. |
|
@vincenospam How did you run upon this one? |
|
@philips We're trying to send JSON-like data, which has, essentially, nested dicts and arrays. We noticed crashes (DBus connection getting closed) that were data dependent. A pattern emerged that the crash would occur when parts of the data would change lengths by increments of 4 bytes. E.g., would be fine 0-3 bytes, would crash 4-7 bytes, ok again for 8-11 bytes, etc. I isolated the behavior down to the encoder, where the marshalled data could not be decoded by the decoder. |
|
So, apart from the missing alignment for the length this looks ok to me. |
|
Alas, the dumping dbus traffic doesn't work, because the current library implementation cannot roundtrip from decode -> encode. That is, the library can decode what it encodes, but it may not be able to re-encode something it decodes from the bus. The issue is that, when decoding, structs are decoded into []interface{}, but encode does not deal with interface{} well (nor does Signature). |
|
This looks good to me. |
Fix encoding alignment logic.
The newEncoder method is recursive, but alignment logic must be based on
"global" absolute position. This FIX allows recursive calls to set the
initial position.
Unit tests added to verify the problem and fix.
The problem was discovered for nested containers where the start of the
nested container can be on a 4-byte-aligned (but not 8-byte-aligned) boundary.