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

Advance pos_of_buffer_start when writing directly to a writer #725

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

Colecf
Copy link
Contributor

@Colecf Colecf commented May 4, 2024

If a message is bigger than the buffer, it will be written directly to the writer, and there was a bug in that case where pos_of_buffer_start was not advanced. This causes later assertions to fail, such as the one in
Message::write_length_delimited_to, which asserts based on total_bytes_written(), which is in turn based on
pos_of_buffer_start.

@stepancheg
Copy link
Owner

Can you please add a test reproducing the issue?

@Colecf
Copy link
Contributor Author

Colecf commented Jun 17, 2024

I couldn't really figure out rust-protobuf's test setup (I couldn't find any documentation either), but I did make a minimal reproduction here: https://github.com/Colecf/rust-protobuf-assertion-failure-reproduction

@stepancheg
Copy link
Owner

rust-protobuf's test setup

Well, it is not easy. Basically, you create two files in

test-crates/protobuf-codegen-protoc-test/src/common/v2/test_xxx_pb.proto
test-crates/protobuf-codegen-protoc-test/src/common/v2/test_xxx.rs

And run cargo test. That will copy these files into three places (v3; v2/v2 with pure codegen).

Alternatively, maybe you can write a simple unit test using CodedOutputStream calls?

@Colecf Colecf force-pushed the fix_write_raw_bytes branch from 7bb9586 to 349da28 Compare June 19, 2024 22:16
If a message is bigger than the buffer, it will be written
directly to the writer, and there was a bug in that case where
pos_of_buffer_start was not advanced. This causes later
assertions to fail, such as the one in
Message::write_length_delimited_to, which asserts based on
total_bytes_written(), which is in turn based on
pos_of_buffer_start.
@Colecf Colecf force-pushed the fix_write_raw_bytes branch from 349da28 to 86dd0f6 Compare June 19, 2024 22:17
@Colecf
Copy link
Contributor Author

Colecf commented Jun 19, 2024

Alternatively, maybe you can write a simple unit test using CodedOutputStream calls?

Ah good point, I added a unit test.

@stepancheg stepancheg merged commit c0a3b0d into stepancheg:master Jun 26, 2024
9 of 10 checks passed
@stepancheg
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants