From 39e68afc1a76f5e2ee19405bd32de7b335d4fb43 Mon Sep 17 00:00:00 2001 From: rsgowman Date: Tue, 1 May 2018 16:33:35 -0400 Subject: Add error handler for serializer (for deserializing cases) (#1181) This is more interesting than the serializing case, as we should expect to see occasional corruption of our input byte vector. --- .../src/firebase/firestore/remote/serializer.cc | 155 +++++++++++++++------ .../src/firebase/firestore/remote/serializer.h | 18 ++- 2 files changed, 126 insertions(+), 47 deletions(-) (limited to 'Firestore/core/src/firebase/firestore') diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index b5a0720..2a89515 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -34,6 +34,7 @@ namespace remote { using firebase::firestore::model::FieldValue; using firebase::firestore::model::ObjectValue; using firebase::firestore::util::Status; +using firebase::firestore::util::StatusOr; namespace { @@ -43,7 +44,7 @@ class Reader; void EncodeObject(Writer* writer, const ObjectValue& object_value); -ObjectValue DecodeObject(Reader* reader); +ObjectValue::Map DecodeObject(Reader* reader); /** * Represents a nanopb tag. @@ -196,6 +197,14 @@ class Reader { return stream_.bytes_left; } + Status status() const { + return status_; + } + + void set_status(Status status) { + status_ = status; + } + private: /** * Creates a new Reader, based on the given nanopb pb_istream_t. Note that @@ -220,6 +229,8 @@ class Reader { */ uint64_t ReadVarint(); + Status status_ = Status::OK(); + pb_istream_t stream_; }; @@ -270,12 +281,17 @@ void Writer::WriteTag(Tag tag) { Tag Reader::ReadTag() { Tag tag; + if (!status_.ok()) return tag; + bool eof; - bool ok = pb_decode_tag(&stream_, &tag.wire_type, &tag.field_number, &eof); - if (!ok || eof) { - // TODO(rsgowman): figure out error handling - abort(); + if (!pb_decode_tag(&stream_, &tag.wire_type, &tag.field_number, &eof)) { + status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_)); + return tag; } + + // nanopb code always returns a false status when setting eof. + FIREBASE_ASSERT_MESSAGE(!eof, "nanopb set both ok status and eof to true"); + return tag; } @@ -301,10 +317,11 @@ void Writer::WriteVarint(uint64_t value) { * @return The decoded varint as a uint64_t. */ uint64_t Reader::ReadVarint() { - uint64_t varint_value; + if (!status_.ok()) return 0; + + uint64_t varint_value = 0; if (!pb_decode_varint(&stream_, &varint_value)) { - // TODO(rsgowman): figure out error handling - abort(); + status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_)); } return varint_value; } @@ -315,9 +332,11 @@ void Writer::WriteNull() { void Reader::ReadNull() { uint64_t varint = ReadVarint(); + if (!status_.ok()) return; + if (varint != google_protobuf_NullValue_NULL_VALUE) { - // TODO(rsgowman): figure out error handling - abort(); + status_ = Status(FirestoreErrorCode::DataLoss, + "Input proto bytes cannot be parsed (invalid null value)"); } } @@ -327,14 +346,18 @@ void Writer::WriteBool(bool bool_value) { bool Reader::ReadBool() { uint64_t varint = ReadVarint(); + if (!status_.ok()) return false; + switch (varint) { case 0: return false; case 1: return true; default: - // TODO(rsgowman): figure out error handling - abort(); + status_ = + Status(FirestoreErrorCode::DataLoss, + "Input proto bytes cannot be parsed (invalid bool value)"); + return false; } } @@ -357,17 +380,21 @@ void Writer::WriteString(const std::string& string_value) { } std::string Reader::ReadString() { + if (!status_.ok()) return ""; + pb_istream_t substream; if (!pb_make_string_substream(&stream_, &substream)) { - // TODO(rsgowman): figure out error handling - abort(); + status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_)); + pb_close_string_substream(&stream_, &substream); + return ""; } std::string result(substream.bytes_left, '\0'); if (!pb_read(&substream, reinterpret_cast(&result[0]), substream.bytes_left)) { - // TODO(rsgowman): figure out error handling - abort(); + status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_)); + pb_close_string_substream(&stream_, &substream); + return ""; } // NB: future versions of nanopb read the remaining characters out of the @@ -375,10 +402,9 @@ std::string Reader::ReadString() { // check within pb_close_string_substream. Unfortunately, that's not present // in the current version (0.38). We'll make a stronger assertion and check // to make sure there *are* no remaining characters in the substream. - if (substream.bytes_left != 0) { - // TODO(rsgowman): figure out error handling - abort(); - } + FIREBASE_ASSERT_MESSAGE( + substream.bytes_left == 0, + "Bytes remaining in substream after supposedly reading all of them."); pb_close_string_substream(&stream_, &substream); @@ -431,29 +457,52 @@ void EncodeFieldValueImpl(Writer* writer, const FieldValue& field_value) { FieldValue DecodeFieldValueImpl(Reader* reader) { Tag tag = reader->ReadTag(); + if (!reader->status().ok()) return FieldValue::NullValue(); // Ensure the tag matches the wire type - // TODO(rsgowman): figure out error handling switch (tag.field_number) { case google_firestore_v1beta1_Value_null_value_tag: case google_firestore_v1beta1_Value_boolean_value_tag: case google_firestore_v1beta1_Value_integer_value_tag: if (tag.wire_type != PB_WT_VARINT) { - abort(); + reader->set_status( + Status(FirestoreErrorCode::DataLoss, + "Input proto bytes cannot be parsed (mismatch between " + "the wiretype and the field number (tag))")); } break; case google_firestore_v1beta1_Value_string_value_tag: case google_firestore_v1beta1_Value_map_value_tag: if (tag.wire_type != PB_WT_STRING) { - abort(); + reader->set_status( + Status(FirestoreErrorCode::DataLoss, + "Input proto bytes cannot be parsed (mismatch between " + "the wiretype and the field number (tag))")); } break; default: - abort(); + // 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. + FIREBASE_ASSERT_MESSAGE( + false, + "Unhandled message field number (tag): %i. (Or possibly " + "corrupt input bytes)", + tag.field_number); + reader->set_status(Status( + FirestoreErrorCode::DataLoss, + "Input proto bytes cannot be parsed (invalid field number (tag))")); } + if (!reader->status().ok()) return FieldValue::NullValue(); + switch (tag.field_number) { case google_firestore_v1beta1_Value_null_value_tag: reader->ReadNull(); @@ -465,12 +514,15 @@ FieldValue DecodeFieldValueImpl(Reader* reader) { case google_firestore_v1beta1_Value_string_value_tag: return FieldValue::StringValue(reader->ReadString()); case google_firestore_v1beta1_Value_map_value_tag: - return FieldValue::ObjectValueFromMap( - DecodeObject(reader).internal_value); + return FieldValue::ObjectValueFromMap(DecodeObject(reader)); default: - // TODO(rsgowman): figure out error handling - abort(); + // This indicates an internal error as we've already ensured that this is + // a valid field_number. + FIREBASE_ASSERT_MESSAGE( + false, + "Somehow got an unexpected field number (tag) after verifying that " + "the field number was expected."); } } @@ -533,24 +585,34 @@ template T Reader::ReadNestedMessage(const std::function& read_message_fn) { // Implementation note: This is roughly modeled on pb_decode_delimited, // adjusted to account for the oneof in FieldValue. + + if (!status_.ok()) return T(); + pb_istream_t raw_substream; if (!pb_make_string_substream(&stream_, &raw_substream)) { - // TODO(rsgowman): figure out error handling - abort(); + status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_)); + pb_close_string_substream(&stream_, &raw_substream); + return T(); } Reader substream(raw_substream); + // If this fails, we *won't* return right away so that we can cleanup the + // substream (although technically, that turns out not to matter; no resource + // leaks occur if we don't do this.) + // TODO(rsgowman): Consider RAII here. (Watch out for Reader class which also + // wraps streams.) T message = read_message_fn(&substream); + status_ = substream.status(); // NB: future versions of nanopb read the remaining characters out of the // substream (and return false if that fails) as an additional safety // check within pb_close_string_substream. Unfortunately, that's not present // in the current version (0.38). We'll make a stronger assertion and check // to make sure there *are* no remaining characters in the substream. - if (substream.bytes_left() != 0) { - // TODO(rsgowman): figure out error handling - abort(); - } + FIREBASE_ASSERT_MESSAGE( + substream.bytes_left() == 0, + "Bytes remaining in substream after supposedly reading all of them."); + pb_close_string_substream(&stream_, &substream.stream_); return message; @@ -591,6 +653,7 @@ void EncodeFieldsEntry(Writer* writer, const ObjectValue::Map::value_type& kv) { ObjectValue::Map::value_type DecodeFieldsEntry(Reader* reader) { Tag tag = reader->ReadTag(); + if (!reader->status().ok()) return {}; // TODO(rsgowman): figure out error handling: We can do better than a failed // assertion. @@ -600,6 +663,7 @@ ObjectValue::Map::value_type DecodeFieldsEntry(Reader* reader) { std::string key = reader->ReadString(); tag = reader->ReadTag(); + if (!reader->status().ok()) return {}; FIREBASE_ASSERT(tag.field_number == google_firestore_v1beta1_MapValue_FieldsEntry_value_tag); FIREBASE_ASSERT(tag.wire_type == PB_WT_STRING); @@ -607,7 +671,7 @@ ObjectValue::Map::value_type DecodeFieldsEntry(Reader* reader) { FieldValue value = reader->ReadNestedMessage(DecodeFieldValueImpl); - return {key, value}; + return ObjectValue::Map::value_type{key, value}; } void EncodeObject(Writer* writer, const ObjectValue& object_value) { @@ -622,12 +686,17 @@ void EncodeObject(Writer* writer, const ObjectValue& object_value) { }); } -ObjectValue DecodeObject(Reader* reader) { - ObjectValue::Map internal_value = reader->ReadNestedMessage( +ObjectValue::Map DecodeObject(Reader* reader) { + if (!reader->status().ok()) return ObjectValue::Map(); + + return reader->ReadNestedMessage( [](Reader* reader) -> ObjectValue::Map { ObjectValue::Map result; + if (!reader->status().ok()) return result; + while (reader->bytes_left()) { Tag tag = reader->ReadTag(); + if (!reader->status().ok()) return result; FIREBASE_ASSERT(tag.field_number == google_firestore_v1beta1_MapValue_fields_tag); FIREBASE_ASSERT(tag.wire_type == PB_WT_STRING); @@ -640,6 +709,7 @@ ObjectValue DecodeObject(Reader* reader) { // map. // TODO(rsgowman): figure out error handling: We can do better than a // failed assertion. + if (!reader->status().ok()) return result; FIREBASE_ASSERT(result.find(fv.first) == result.end()); // Add this key,fieldvalue to the results map. @@ -647,7 +717,6 @@ ObjectValue DecodeObject(Reader* reader) { } return result; }); - return ObjectValue{internal_value}; } } // namespace @@ -659,9 +728,15 @@ Status Serializer::EncodeFieldValue(const FieldValue& field_value, return writer.status(); } -FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) { +StatusOr Serializer::DecodeFieldValue(const uint8_t* bytes, + size_t length) { Reader reader = Reader::Wrap(bytes, length); - return DecodeFieldValueImpl(&reader); + FieldValue fv = DecodeFieldValueImpl(&reader); + if (reader.status().ok()) { + return fv; + } else { + return reader.status(); + } } } // namespace remote diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.h b/Firestore/core/src/firebase/firestore/remote/serializer.h index 7f08f7d..7d13583 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.h +++ b/Firestore/core/src/firebase/firestore/remote/serializer.h @@ -24,6 +24,7 @@ #include "Firestore/core/src/firebase/firestore/model/field_value.h" #include "Firestore/core/src/firebase/firestore/util/firebase_assert.h" #include "Firestore/core/src/firebase/firestore/util/status.h" +#include "Firestore/core/src/firebase/firestore/util/statusor.h" #include "absl/base/attributes.h" namespace firebase { @@ -66,8 +67,9 @@ class Serializer { * @param field_value the model to convert. * @param[out] out_bytes A buffer to place the output. The bytes will be * appended to this vector. + * @return A Status, which if not ok(), indicates what went wrong. Note that + * errors during encoding generally indicate a serious/fatal error. */ - // TODO(rsgowman): error handling, incl return code. // TODO(rsgowman): If we never support any output except to a vector, it may // make sense to have Serializer own the vector and provide an accessor rather // than asking the user to create it first. @@ -80,20 +82,22 @@ class Serializer { * * @param bytes The bytes to convert. It's assumed that exactly all of the * bytes will be used by this conversion. - * @return The model equivalent of the bytes. + * @return The model equivalent of the bytes or a Status indicating + * what went wrong. */ - // TODO(rsgowman): error handling. - model::FieldValue DecodeFieldValue(const uint8_t* bytes, size_t length); + util::StatusOr DecodeFieldValue(const uint8_t* bytes, + size_t length); /** * @brief Converts from bytes to the model FieldValue format. * * @param bytes The bytes to convert. It's assumed that exactly all of the * bytes will be used by this conversion. - * @return The model equivalent of the bytes. + * @return The model equivalent of the bytes or a Status indicating + * what went wrong. */ - // TODO(rsgowman): error handling. - model::FieldValue DecodeFieldValue(const std::vector& bytes) { + util::StatusOr DecodeFieldValue( + const std::vector& bytes) { return DecodeFieldValue(bytes.data(), bytes.size()); } -- cgit v1.2.3