Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Marshaling into already allocated proto.Buffer stopped working #619

Open
jrouviere opened this issue Sep 6, 2019 · 4 comments
Open

Marshaling into already allocated proto.Buffer stopped working #619

jrouviere opened this issue Sep 6, 2019 · 4 comments
Labels
Milestone

Comments

@jrouviere
Copy link

jrouviere commented Sep 6, 2019

Marshalling into an already allocated proto.Buffer stopped working after the merge of #560.

For instance:

var msg proto.Message

// using a proto.Buffer:
buf := proto.NewBuffer(make([]byte, 0, 1024))
buf.Marshal(msg)
dataBuf := buf.Bytes()

// using direct proto.Marshal:
dataMarshal, _ := proto.Marshal(msg)

// if the encoded message is smaller than 1024, dataBuf is invalid
bytes.Equal(dataBuf, dataMarshal) // false

I believe the issue is caused by the generated XXX_Marshal methods, which now expect the capacity of the buffer to be exactly the size of the message.

The first time a proto.Buffer object is used its capacity will be set to the size of the message, the second time it is used, and if the message to be marshalled is smaller, the capacity stay the same and is now higher than the length of the message.

The problem is that XXX_Marshal force the size of the buffer to its capacity (b = b[:cap(b)]), and then truncate it back to the actual marshalled size (return b[:n], nil), this behaviour was fine before #560.
Now that we marshal backwards, the data is written at the end of the buffer (at full capacity), and XXX_Marshal returns a slice at the beginning of the buffer containing uninitialised data.

@jrouviere
Copy link
Author

Replacing
b = b[:cap(b)]
by
b = b[:m.Size()]
in protoc-gen-gogo/generator/generator.go seems to fix the issue, but I'm not sure about the wider impact.

@jmarais
Copy link
Contributor

jmarais commented Sep 7, 2019

Thanks for the report.
@apelisse It seems like we will have to call MarshalTo instead of MarshalToSizedBuffer in the XXX_Marshal method.
This cause at least two Size() calls for marshaling any message. Do you think we can do anything better here?

jmarais added a commit to jmarais/protobuf that referenced this issue Sep 7, 2019
…en a proto.Buffer and proto.Marshal with different proto.Buffer sizes.
@jmarais jmarais added the bug label Sep 7, 2019
@jmarais jmarais added this to the v1.3.0 milestone Sep 7, 2019
jmarais added a commit to jmarais/protobuf that referenced this issue Sep 8, 2019
…. This allows us to marshal at the correct place when the buffer contains a larger backing buffer. (gogo#619)
@jmarais
Copy link
Contributor

jmarais commented Sep 8, 2019

@jrouviere
Can you please let me know if:
https://github.com/jmarais/protobuf/tree/issue619
fixes your problem.

@jrouviere
Copy link
Author

@jmarais, Yes your branch fixes the problem I was observing. Awesome work, thanks for your quick response!

apelisse pushed a commit to apelisse/protobuf that referenced this issue Sep 9, 2019
…en a proto.Buffer and proto.Marshal with different proto.Buffer sizes.
jmarais added a commit to jmarais/protobuf that referenced this issue Sep 17, 2019
jmarais added a commit that referenced this issue Oct 11, 2019
* tests. added test case for issue (#619). Compare marshaling between a proto.Buffer and proto.Marshal with different proto.Buffer sizes.

* proto/buffer. added buffer cap adjustment before giving it to marshal. This allows us to marshal at the correct place when the buffer contains a larger backing buffer. (#619)

* proto/buffer. remove unsafe/reflect slice cap adjustment and rather use a full slice expression

* proto/buffer. encode. add issue620 tests here

* proto/buffer encode. removed  unnecessary slicing. (#619)

* proto/buffer.  err check previously ignored error

* issue619. add test package to make regenerate list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants