Parsing a protobuf message doesn't properly skip unknown fields

[dmitry-timin]

While deserializing a protobuf message, BeanDeserializercalls ProtobufParser.nextFieldName() in a loop. Following piece of code looks for a field by id, which should be equal to "index" from message schema:

            int tag = _decodeVInt();
            // inlined _handleRootKey()

            int wireType = (tag & 0x7);
            int id = (tag >> 3);

            ProtobufField f = _findField(id);
            if (f == null) {
                if (_skipUnknownField(id, wireType) != JsonToken.FIELD_NAME) {
                    return null;
                }

here, if _findField(id) returns null (in case message contains unknown field), _skipUnknownField skips the whole field in the stream. Problem is that after unknown field is skipped, local variables tag, wireType, id are still contain info about skipped field and used directly afterwards, while current field is already next one.

Called function _skipUnknownField(id, wireType) already reads next fields tag and sets class global variable _currentField to next field, so what has to be done in caller's code is to update local variables to next valid field:

            ProtobufField f = _findField(id);
            if (f == null) {
                if (_skipUnknownField(id, wireType) != JsonToken.FIELD_NAME) {
                    return null;
                }
                // sub-optimal as skip method already set it, but:

                // update local variables to next field after unknown field is skipped
                tag = _currentField.typedTag;
                wireType = _currentField.wireType;
                id = _currentField.id;
            }

[cowtowncoder]

Thank you for reporting this problem. I know it may be tricky to do, but is there any change to have a failing unit test to reproduce? (if not, that is ok; would just be great to have it against regressions)

[dmitry-timin]

ok I will prepare testcase

[dmitry-timin]

Testcase is ready. it should fail with original sources and pass with fix. important: class fields should be ordered alphabetically, which breaks ordering by index and unknown fields appear in between known fields in the stream.

Please note that ProtobufParser uses similar code to skip unknown fields in several places, not covered by this testcase, for example in another function public boolean nextFieldName(SerializableString sstr) throws IOException

testcase.zip

[cowtowncoder]

@dmitry-timin Thank you very much for reporting this, adding reproduction. I think I fixed it in necessary places (2 out of 4 did not retain local state to update); fix will be in 2.10.4 and 2.11.0.