From 26b8ac9ee43bb67b08e4bc62af98ac6bda2f121c Mon Sep 17 00:00:00 2001 From: rsgowman Date: Mon, 4 Jun 2018 15:31:10 -0400 Subject: Skip unknown fields while decoding FieldValue proto objects. (#1354) --- .../core/src/firebase/firestore/nanopb/reader.cc | 8 +++ .../core/src/firebase/firestore/nanopb/reader.h | 9 +++ .../src/firebase/firestore/remote/serializer.cc | 40 +++++++------ .../firebase/firestore/remote/serializer_test.cc | 65 ++++++++++++++++++---- 4 files changed, 95 insertions(+), 27 deletions(-) (limited to 'Firestore/core') diff --git a/Firestore/core/src/firebase/firestore/nanopb/reader.cc b/Firestore/core/src/firebase/firestore/nanopb/reader.cc index ec4282d..69e3d83 100644 --- a/Firestore/core/src/firebase/firestore/nanopb/reader.cc +++ b/Firestore/core/src/firebase/firestore/nanopb/reader.cc @@ -136,6 +136,14 @@ std::string Reader::ReadString() { return result; } +void Reader::SkipField(const Tag& tag) { + if (!status_.ok()) return; + + if (!pb_skip_field(&stream_, tag.wire_type)) { + status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_)); + } +} + } // namespace nanopb } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/firebase/firestore/nanopb/reader.h b/Firestore/core/src/firebase/firestore/nanopb/reader.h index 930211a..2c16ec4 100644 --- a/Firestore/core/src/firebase/firestore/nanopb/reader.h +++ b/Firestore/core/src/firebase/firestore/nanopb/reader.h @@ -89,6 +89,15 @@ class Reader { template T ReadNestedMessage(const std::function& read_message_fn); + /** + * Discards the bytes associated with the given tag. + * + * @param tag The tag associated with the field that is otherwise about to be + * read. This method uses the tag to determine how many bytes should be + * discarded. + */ + void SkipField(const Tag& tag); + size_t bytes_left() const { return stream_.bytes_left; } diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index 0839efc..eee56f8 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -204,20 +204,20 @@ FieldValue DecodeFieldValueImpl(Reader* reader) { } break; - default: - // We could get here for one of two reasons; either because the input - // bytes are corrupt, or because we're attempting to parse a tag that we - // haven't implemented yet. Long term, the latter reason should become - // less likely (especially in production), so we'll assume former. - - // TODO(rsgowman): While still in development, we'll contradict the - // above and assume the latter. Remove the following assertion when - // we're confident that we're handling all the tags in the protos. + case google_firestore_v1beta1_Value_double_value_tag: + case google_firestore_v1beta1_Value_bytes_value_tag: + case google_firestore_v1beta1_Value_reference_value_tag: + case google_firestore_v1beta1_Value_geo_point_value_tag: + case google_firestore_v1beta1_Value_array_value_tag: + // TODO(b/74243929): Implement remaining types. HARD_FAIL("Unhandled message field number (tag): %i.", tag.field_number); - reader->set_status(Status( - FirestoreErrorCode::DataLoss, - "Input proto bytes cannot be parsed (invalid field number (tag))")); + + default: + // Unknown tag. According to the proto spec, we need to ignore these. No + // action required here, though we'll need to skip the relevant bytes + // below. + break; } if (!reader->status().ok()) return FieldValue::NullValue(); @@ -247,12 +247,18 @@ FieldValue DecodeFieldValueImpl(Reader* reader) { reader->ReadNestedMessage(DecodeMapValue)); break; + case google_firestore_v1beta1_Value_double_value_tag: + case google_firestore_v1beta1_Value_bytes_value_tag: + case google_firestore_v1beta1_Value_reference_value_tag: + case google_firestore_v1beta1_Value_geo_point_value_tag: + case google_firestore_v1beta1_Value_array_value_tag: + // TODO(b/74243929): Implement remaining types. + HARD_FAIL("Unhandled message field number (tag): %i.", + tag.field_number); + default: - // This indicates an internal error as we've already ensured that this - // is a valid field_number. - HARD_FAIL( - "Somehow got an unexpected field number (tag) after verifying that " - "the field number was expected."); + // Unknown tag. According to the proto spec, we need to ignore these. + reader->SkipField(tag); } } diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc index 96ffa9e..7dcdf6e 100644 --- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc +++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc @@ -605,7 +605,12 @@ TEST_F(SerializerTest, BadTimestampValue_TooSmall) { Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } -TEST_F(SerializerTest, BadTag) { +TEST_F(SerializerTest, BadFieldValueTagAndNoOtherTagPresent) { + // A bad tag should be ignored. But if there are *no* valid tags, then we + // don't know the type of the FieldValue. Although it might be reasonable to + // assume some sort of default type in this situation, we've decided to fail + // the deserialization process in this case instead. + std::vector bytes = EncodeFieldValue(&serializer, FieldValue::NullValue()); @@ -615,15 +620,55 @@ TEST_F(SerializerTest, BadTag) { // Specifically 31. 0xf8 represents field number 31 encoded as a varint. Mutate(&bytes[0], /*expected_initial_value=*/0x58, /*new_value=*/0xf8); - // TODO(rsgowman): The behaviour is *temporarily* slightly different during - // development; this will cause a failed assertion rather than a failed - // status. Remove this EXPECT_ANY_THROW statement (and reenable the - // following commented out statement) once the corresponding assert has been - // removed from serializer.cc. - EXPECT_ANY_THROW(ExpectFailedStatusDuringFieldValueDecode( - Status(FirestoreErrorCode::DataLoss, "ignored"), bytes)); - // ExpectFailedStatusDuringFieldValueDecode( - // Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); + ExpectFailedStatusDuringFieldValueDecode( + Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); +} + +TEST_F(SerializerTest, BadFieldValueTagWithOtherValidTagsPresent) { + // A bad tag should be ignored, in which case, we should successfully + // deserialize the rest of the bytes as if it wasn't there. To craft these + // bytes, we'll use the same technique as + // EncodesFieldValuesWithRepeatedEntries (so go read the comments there + // first). + + // Copy of the real one (from the nanopb generated document.pb.h), but with + // only boolean_value and integer_value. + struct google_firestore_v1beta1_Value_Fake { + bool boolean_value; + int64_t integer_value; + }; + + // Copy of the real one (from the nanopb generated document.pb.c), but with + // only boolean_value and integer_value. Also modified such that integer_value + // now has an invalid tag (instead of 2). + const int invalid_tag = 31; + const pb_field_t google_firestore_v1beta1_Value_fields_Fake[2] = { + PB_FIELD(1, BOOL, SINGULAR, STATIC, FIRST, + google_firestore_v1beta1_Value_Fake, boolean_value, + boolean_value, 0), + PB_FIELD(invalid_tag, INT64, SINGULAR, STATIC, OTHER, + google_firestore_v1beta1_Value_Fake, integer_value, + boolean_value, 0), + }; + + // Craft the bytes. boolean_value has a smaller tag, so it'll get encoded + // first, normally implying integer_value should "win". Except that + // integer_value isn't a valid tag, so it should be ignored here. + google_firestore_v1beta1_Value_Fake crafty_value{false, int64_t{42}}; + std::vector bytes(128); + pb_ostream_t stream = pb_ostream_from_buffer(bytes.data(), bytes.size()); + pb_encode(&stream, google_firestore_v1beta1_Value_fields_Fake, &crafty_value); + bytes.resize(stream.bytes_written); + + // Decode the bytes into the model + StatusOr actual_model_status = serializer.DecodeFieldValue(bytes); + EXPECT_OK(actual_model_status); + FieldValue actual_model = actual_model_status.ValueOrDie(); + + // Ensure the decoded model is as expected. + FieldValue expected_model = FieldValue::BooleanValue(false); + EXPECT_EQ(FieldValue::Type::Boolean, actual_model.type()); + EXPECT_EQ(expected_model, actual_model); } TEST_F(SerializerTest, TagVarintWiretypeStringMismatch) { -- cgit v1.2.3