Skip to content

Chunk oversized JSON string writes in JsonToBinaryStream#26911

Open
Amemoyoi wants to merge 5 commits intoprotocolbuffers:mainfrom
Amemoyoi:fix/json-to-binary-intmax-v2
Open

Chunk oversized JSON string writes in JsonToBinaryStream#26911
Amemoyoi wants to merge 5 commits intoprotocolbuffers:mainfrom
Amemoyoi:fix/json-to-binary-intmax-v2

Conversation

@Amemoyoi
Copy link
Copy Markdown

This change avoids oversized raw writes in the proto3 JSON-to-binary streaming path.

Changes:

  • chunk oversized writes in ParseProto3Type::SetString() instead of passing a size_t length directly into WriteRaw(const void*, int)
  • add a focused regression test for the chunking behavior at the INT_MAX + 1 boundary

@esrauchg
Copy link
Copy Markdown
Contributor

esrauchg commented Apr 16, 2026

Thank you for raising this!

Serializing strings above 2 GiB is not supported by protobuf in general (our length-prefix format has an int32 for sizes, so strings that large aren't representable in our main format at all).

I think we likely would prefer to serialize-fail if such a condition is hit rather than trying to gracefully chunk up the writes: do you think you could update the PR to do?

@Amemoyoi
Copy link
Copy Markdown
Author

Thanks, that makes sense.
I’ve updated the PR to fail cleanly when a JSON string/bytes value exceeds the supported INT32_MAX serialization limit, instead of chunking the writes.

Copy link
Copy Markdown
Contributor

@esrauchg esrauchg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Comment thread src/google/protobuf/json/internal/parser_traits.h
@esrauchg esrauchg added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Apr 16, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Apr 16, 2026
@Amemoyoi Amemoyoi requested a review from esrauchg April 19, 2026 04:50
Copy link
Copy Markdown
Contributor

@esrauchg esrauchg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@esrauchg esrauchg added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Apr 21, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Apr 21, 2026
@Amemoyoi
Copy link
Copy Markdown
Author

Thanks again for the review.
I’ve synced the branch with main, and the blocking checks appear to be passing now.
Please let me know if there’s anything else needed from my side before merge.

@esrauchg esrauchg added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Apr 23, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Apr 23, 2026
@esrauchg
Copy link
Copy Markdown
Contributor

Unfortunately looks like theres a build breakage with latest proposed change, are you able to fix?

Thanks!

@Amemoyoi
Copy link
Copy Markdown
Author

Thanks for the heads up. The breakage was from defining the helper in the header with external linkage. I’ve updated it to be inline and pushed the fix.

@Amemoyoi Amemoyoi requested a review from esrauchg April 25, 2026 08:34
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