BinaryToJsonString/BinaryToJsonStream returns OK on malformed unknown length-delimited field (Skip failure ignored)

[Ky0toFu]

What version of protobuf and what language are you using?
Version: v33.2 (git tag commit 7ccf2060af4cd13c44a8659d0999be6c7a98fcc1)
Language: C++

What operating system (Linux, Windows, ...) and version?
Windows 11 with WSL Ubuntu 24.04 (also reproducible on Linux).

What runtime / compiler are you using (e.g., python version or gcc version)
CMake build on WSL Ubuntu toolchain (gcc/clang). (I can provide exact versions if needed.)

What did you do?
Steps to reproduce the behavior:

1. Build protobuf v33.2.
2. Use a schema like:

syntax = "proto3";
package p;
message M { int32 a = 1; }

3. Call google::protobuf::json::BinaryToJsonString() (or BinaryToJsonStream()) for type type.googleapis.com/p.M with this crafted wire input (hex):

c2 3e 80 80 80 80 08 08 b9 0a

Interpretation:

• c2 3e = unknown field tag (field JSON handling for Any #1000, wire type length-delimited)
• 80 80 80 80 08 = length varint 0x80000000 (= 2147483648 = INT_MAX + 1)
• 08 = tag for known field a (field Added const qualifier to iterator to enable compiling with VS2008 #1, varint)
• b9 0a = varint value 1337

What did you expect to see
Conversion should fail (invalid/malformed wire input). The unknown length-delimited field length cannot be skipped and should be treated as an error.

What did you see instead?
Conversion returns OK and produces “valid-looking” JSON (parse confusion), e.g. {"a":1337}.

Anything else we should know about your project / environment
Root cause in src/google/protobuf/json/internal/untyped_message.cc (UntypedMessage::Decode()), unknown-field handling for WIRETYPE_LENGTH_DELIMITED:

uint32_t x;
if (!stream.ReadVarint32(&x)) {
  return MakeUnexpectedEofError();
}
stream.Skip(x);
continue;

CodedInputStream::Skip(int) takes an int. For x > INT_MAX, implicit conversion yields a negative value, Skip() returns false, but the return value is ignored so parsing continues.

Permalink (v33.2):

protobuf/src/google/protobuf/json/internal/untyped_message.cc

Lines 254 to 260 in 7ccf206

	case WireFormatLite::WIRETYPE_LENGTH_DELIMITED: {
	uint32_t x;
	if (!stream.ReadVarint32(&x)) {
	return MakeUnexpectedEofError();
	}
	stream.Skip(x);
	continue;

Suggested fix:

• Reject x > INT_MAX.
• Check the return value of Skip() and return an error when it fails.

[esrauchg]

Thanks for reporting this.

We'll look into this, but I think it might just be spec (or common and 'allowable-behavior') that length-prefixes are coerced to int32 before resolution.

It wouldn't be a 'good' behavior necessarily, but the general varint parsing behavior in general is coerce to target type (if you had UINT32_MAX+1 on the wire as a varint for an uint32 field, spec is that it should parse into that field as 0 length).

Recently we clarified that overlarge varints aren't allowed for field numbers and the behavior is currently inconsistent for when that is or isn't accepted. It might be that we should declare it 'wrong', but it would be a breaking change to fix in the case of length-prefixes since customers may have this bad-shaped data and have it successfully handled by their system today (flipping strange but has-always-parsed data to unparsable is not something we do lightly because it can be very difficult for people to remediate some of their stored data).

[Ky0toFu]

Thanks — that makes sense re: compatibility concerns.

One nuance: length-prefixes are framing, so “coerce-to-target-type” semantics (common for numeric fields) can change how subsequent bytes are interpreted structurally. In this specific code path we read uint32_t x via ReadVarint32() (so x = 0x80000000 is representable as uint32_t), and then pass it to CodedInputStream::Skip(int). For x > INT_MAX, the implicit uint32_t -> int conversion (when the value is not representable) is implementation-defined in C++, and Skip() fails (returns false), but the return value is ignored. So the current behavior seems to depend on a platform-dependent conversion + ignored failure, rather than an explicit/documented coercion rule.

Even if the project decides length-prefix values > INT_MAX should be “allowable” for compatibility, could we make the behavior explicit and consistent, and at minimum check the Skip() return value and handle failure in a well-defined way?

[esrauchg]

Yes, looking closer at this versus the main unknown field skip path here:
https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/wire_format.cc#L87-L90

The semantic on that path is if the varint is INT32_MAX < x < UINT32_MAX it will fail in the Skip() call (which means, the 32nd bit in the varint must not be set) but it will silently discard bits higher than that. Which is a somewhat weird semantic, but a simple bug that the Skip() return value isn't checked in this path, and though it in theory could flip some parse-success to parse-fails I think it is sufficiently 'clearly a bug' and misaligned with the main path that harmonizing won't be a breaking change (if we get it in today it will go with the 34 release which is incidentally a breaking change release in C++ anyway too).

I'll validate that this fix doesn't break any projects internally and send the PR as long at doesn't.

Thank you for reporting this and helping clarify!

[esrauchg]

Thank you again for the report, the fix to not-ignore the Skip() return value go out with our 34 release.