From 0f2c638b773ffd23308f44413e7f8bddb5fa0986 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Wed, 1 Jul 2026 15:16:10 -0700 Subject: [PATCH] Fix #598: (protobuf) Protobuf parser state handling wrong for implicit close (END_OBJECT) Add STATE_ROOT_END so that the parser separates returning the implicit root-level END_OBJECT from actually closing the underlying input/parser, matching jackson-core#1438 semantics. Also add integer-overflow guards for packed-array and message length decoding, and fix ProtobufField.compareTo() to use Integer.compare() instead of subtraction. --- .../dataformat/protobuf/ProtobufParser.java | 35 +++++- .../protobuf/schema/ProtobufField.java | 2 +- .../protobuf/ParserStateEndTest.java | 116 ++++++++++++++++++ release-notes/VERSION-2.x | 5 + 4 files changed, 152 insertions(+), 6 deletions(-) create mode 100644 protobuf/src/test/java/com/fasterxml/jackson/dataformat/protobuf/ParserStateEndTest.java diff --git a/protobuf/src/main/java/com/fasterxml/jackson/dataformat/protobuf/ProtobufParser.java b/protobuf/src/main/java/com/fasterxml/jackson/dataformat/protobuf/ProtobufParser.java index e13d71c62..dee473f1b 100644 --- a/protobuf/src/main/java/com/fasterxml/jackson/dataformat/protobuf/ProtobufParser.java +++ b/protobuf/src/main/java/com/fasterxml/jackson/dataformat/protobuf/ProtobufParser.java @@ -51,6 +51,10 @@ public class ProtobufParser extends ParserMinimalBase // State after either reaching end-of-input, or getting explicitly closed private final static int STATE_CLOSED = 12; + // State after returning END_OBJECT for root level, before closing + // (added for [dataformats-binary#598] to separate END_OBJECT return from close()) + private final static int STATE_ROOT_END = 13; + private final static int[] UTF8_UNIT_CODES = ProtobufUtil.sUtf8UnitLengths; // @since 2.14 @@ -592,7 +596,8 @@ public JsonToken nextToken() throws IOException // end-of-input? if (_inputPtr >= _inputEnd) { if (!loadMore()) { - close(); + _state = STATE_ROOT_END; + _parsingContext.setCurrentName(null); return _updateToken(JsonToken.END_OBJECT); } } @@ -617,6 +622,12 @@ public JsonToken nextToken() throws IOException int len = _decodeLength(); int newEnd = _inputPtr + len; + // Guard against integer overflow: _inputPtr and len are both non-negative, + // so a result smaller than _inputPtr means the sum wrapped. + if (newEnd < _inputPtr) { + _reportErrorF("Packed array length overflows for field '%s': ptr=%d, len=%d", + _currentField.name, _inputPtr, len); + } // First: validate that we do not extend past end offset of enclosing message if (!_parsingContext.inRoot()) { @@ -687,9 +698,14 @@ public JsonToken nextToken() throws IOException return _updateToken(_readNextValue(_currentField.type, STATE_NESTED_KEY)); case STATE_MESSAGE_END: // occurs if we end with array - close(); // sets state to STATE_CLOSED + _state = STATE_ROOT_END; + _parsingContext.setCurrentName(null); return _updateToken(JsonToken.END_OBJECT); + case STATE_ROOT_END: // returned END_OBJECT, now close on next call + close(); // sets state to STATE_CLOSED + return null; + case STATE_CLOSED: return null; @@ -923,6 +939,12 @@ private JsonToken _readNextValue(FieldType t, int nextState) throws IOException _currentMessage = msg; int len = _decodeLength(); int newEnd = _inputPtr + len; + // Guard against integer overflow: _inputPtr and len are both non-negative, + // so a result smaller than _inputPtr means the sum wrapped. + if (newEnd < _inputPtr) { + _reportErrorF("Message length overflows for field '%s': ptr=%d, len=%d", + _currentField.name, _inputPtr, len); + } // First: validate that we do not extend past end offset of enclosing message if (newEnd > _currentEndOffset) { @@ -964,7 +986,8 @@ private JsonToken _skipUnknownField(int tag, int wireType) throws IOException } } else if (_inputPtr >= _inputEnd) { if (!loadMore()) { - close(); + _state = STATE_ROOT_END; + _parsingContext.setCurrentName(null); return _updateToken(JsonToken.END_OBJECT); } } @@ -1025,7 +1048,8 @@ public boolean nextFieldName(SerializableString sstr) throws IOException if (_state == STATE_ROOT_KEY) { if (_inputPtr >= _inputEnd) { if (!loadMore()) { - close(); + _state = STATE_ROOT_END; + _parsingContext.setCurrentName(null); _updateToken(JsonToken.END_OBJECT); return false; } @@ -1106,7 +1130,8 @@ public String nextFieldName() throws IOException if (_state == STATE_ROOT_KEY) { if (_inputPtr >= _inputEnd) { if (!loadMore()) { - close(); + _state = STATE_ROOT_END; + _parsingContext.setCurrentName(null); _updateToken(JsonToken.END_OBJECT); return null; } diff --git a/protobuf/src/main/java/com/fasterxml/jackson/dataformat/protobuf/schema/ProtobufField.java b/protobuf/src/main/java/com/fasterxml/jackson/dataformat/protobuf/schema/ProtobufField.java index 619b5ac1a..66fd8dad3 100644 --- a/protobuf/src/main/java/com/fasterxml/jackson/dataformat/protobuf/schema/ProtobufField.java +++ b/protobuf/src/main/java/com/fasterxml/jackson/dataformat/protobuf/schema/ProtobufField.java @@ -212,6 +212,6 @@ public String toString() // for debugging @Override public int compareTo(ProtobufField other) { - return id - other.id; + return Integer.compare(id, other.id); } } diff --git a/protobuf/src/test/java/com/fasterxml/jackson/dataformat/protobuf/ParserStateEndTest.java b/protobuf/src/test/java/com/fasterxml/jackson/dataformat/protobuf/ParserStateEndTest.java new file mode 100644 index 000000000..159845250 --- /dev/null +++ b/protobuf/src/test/java/com/fasterxml/jackson/dataformat/protobuf/ParserStateEndTest.java @@ -0,0 +1,116 @@ +package com.fasterxml.jackson.dataformat.protobuf; + +import org.junit.jupiter.api.Test; + +import com.fasterxml.jackson.core.*; + +import com.fasterxml.jackson.dataformat.protobuf.schema.ProtobufSchema; +import com.fasterxml.jackson.dataformat.protobuf.schema.ProtobufSchemaLoader; + +import static org.junit.jupiter.api.Assertions.*; + +// [dataformats-binary#598] +public class ParserStateEndTest extends ProtobufTestBase +{ + private final ProtobufMapper MAPPER = newObjectMapper(); + + @Test + public void testParserStateAtEndObject() throws Exception + { + ProtobufSchema schema = ProtobufSchemaLoader.std.parse(PROTOC_POINT); + + Point input = new Point(42, 13); + byte[] bytes = MAPPER.writerFor(Point.class) + .with(schema) + .writeValueAsBytes(input); + + try (JsonParser p = MAPPER.reader() + .with(schema) + .createParser(bytes)) { + assertToken(JsonToken.START_OBJECT, p.nextToken()); + + assertToken(JsonToken.FIELD_NAME, p.nextToken()); + assertEquals("x", p.currentName()); + + assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken()); + assertEquals(42, p.getIntValue()); + + assertToken(JsonToken.FIELD_NAME, p.nextToken()); + assertEquals("y", p.currentName()); + + assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken()); + assertEquals(13, p.getIntValue()); + + assertToken(JsonToken.END_OBJECT, p.nextToken()); + + assertFalse(p.isClosed(), + "Parser should NOT be closed immediately after returning END_OBJECT"); + + assertEquals(JsonToken.END_OBJECT, p.getCurrentToken(), + "currentToken() should return END_OBJECT, not null"); + + assertNull(p.nextToken(), "After END_OBJECT, nextToken() should return null"); + assertTrue(p.isClosed(), "Parser should be closed after nextToken() returns null"); + } + } + + @Test + public void testParserStateAtEndObjectWithNextFieldName() throws Exception + { + ProtobufSchema schema = ProtobufSchemaLoader.std.parse(PROTOC_POINT); + + Point input = new Point(42, 13); + byte[] bytes = MAPPER.writerFor(Point.class) + .with(schema) + .writeValueAsBytes(input); + + try (JsonParser p = MAPPER.reader() + .with(schema) + .createParser(bytes)) { + assertToken(JsonToken.START_OBJECT, p.nextToken()); + + assertEquals("x", p.nextFieldName()); + assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken()); + + assertEquals("y", p.nextFieldName()); + assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken()); + + assertNull(p.nextFieldName()); + + assertEquals(JsonToken.END_OBJECT, p.getCurrentToken(), + "currentToken() should return END_OBJECT after nextFieldName() returns null"); + + assertFalse(p.isClosed(), + "Parser should NOT be closed when currentToken is END_OBJECT"); + + assertNull(p.nextToken()); + assertTrue(p.isClosed()); + } + } + + @Test + public void testParserStateWithEmptyMessage() throws Exception + { + final String PROTOC_EMPTY = "message Empty {}\n"; + ProtobufSchema schema = ProtobufSchemaLoader.std.parse(PROTOC_EMPTY); + + // Empty Protobuf message is legitimately zero bytes: no fields to encode + byte[] bytes = new byte[0]; + + try (JsonParser p = MAPPER.reader() + .with(schema) + .createParser(bytes)) { + assertToken(JsonToken.START_OBJECT, p.nextToken()); + assertFalse(p.isClosed()); + + assertToken(JsonToken.END_OBJECT, p.nextToken()); + + assertFalse(p.isClosed(), + "Parser should NOT be closed immediately after END_OBJECT"); + assertEquals(JsonToken.END_OBJECT, p.getCurrentToken()); + + assertNull(p.nextToken()); + assertTrue(p.isClosed()); + } + } +} diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x index c8f7e7a32..ea1c270bb 100644 --- a/release-notes/VERSION-2.x +++ b/release-notes/VERSION-2.x @@ -14,6 +14,11 @@ Active maintainers: === Releases === ------------------------------------------------------------------------ +2.21.5 (not yet released) + +#598: (protobuf) Protobuf parser state handling wrong for implicit close + (END_OBJECT) + 2.21.4 (28-May-2026) #691: (cbor) Add parameterized tests covering all ASCII-optimization exit paths in CBORParser