fix container partial read bug

Summary:
Fixes a logical error when skipping items in list caused an error in dvaar gateway when skipping items in a list.

A test is added that previous would have caused the program to crash on an assert

Reviewed By: fadimounir

Differential Revision: D65596879

fbshipit-source-id: 4fa100b922ceaf78d2355dcd52a9cf9a1ee0d76b

third-party/thrift/src/thrift/lib/cpp2/protocol/CursorBasedSerializer.h

@@ -708,8 +708,14 @@ class ContainerCursorIterator {
   // Dereference
   // These return non-const to allow moving the deserialzed value out.
   // Modifying them does not affect the underlying buffer.
-  reference operator*() { return reader_->currentValue(); }
-  pointer operator->() { return &reader_->currentValue(); }
+  reference operator*() {
+    FOLLY_SAFE_DCHECK(reader_);
+    return reader_->currentValue();
+  }
+  pointer operator->() {
+    FOLLY_SAFE_DCHECK(reader_);
+    return &reader_->currentValue();
+  }
 
   // Equality is only defined against itself and the end iterator
   bool operator==(ContainerCursorIterator other) const {
@@ -1193,7 +1199,11 @@ void ContainerCursorReader<Tag, ProtocolReader, Contiguous>::finalize() {
   checkState(State::Active);
   state_ = State::Done;
 
-  if (remaining_ > 1) {
+  if (remaining_ > 0) {
+    if (!lastRead_) {
+      lastRead_.emplace();
+      readItem();
+    }
     // Skip remaining unread elements
     skip_n(*protocol_, remaining_ - 1, detail::ContainerTraits<Tag>::wireTypes);
   }

third-party/thrift/src/thrift/lib/cpp2/protocol/Protocol.h

@@ -50,6 +50,9 @@ FOLLY_GFLAGS_DECLARE_int32(thrift_protocol_max_depth);
  */
 
 namespace apache::thrift {
+
+class BinaryProtocolReader;
+
 namespace detail {
 
 class ProtocolBase {
@@ -228,8 +231,14 @@ void skip(Protocol_& prot, WireType arg_type, int depth = 0) {
     case TType::T_UTF8:
     case TType::T_UTF16:
     case TType::T_STRING: {
-      apache::thrift::detail::SkipNoopString str;
-      prot.readBinary(str);
+      if constexpr (std::is_same_v<Protocol_, BinaryProtocolReader>) {
+        int32_t size;
+        prot.readI32(size);
+        prot.skipBytes(size);
+      } else {
+        apache::thrift::detail::SkipNoopString str;
+        prot.readBinary(str);
+      }
       return;
     }
     case TType::T_STRUCT: {

third-party/thrift/src/thrift/lib/cpp2/protocol/test/CursorBasedSerializerTest.cpp

@@ -640,3 +640,49 @@ TEST(CursorSerializer, CursorWriteInContainer) {
       *obj.set_nested_field(),
       Contains(Contains(IsThriftUnionWith<ident::string_field>(Eq("foo")))));
 }
+
+void doCursorReadRemainEndTest(int count) {
+  std::unique_ptr<folly::IOBuf> buf;
+  {
+    ReadRemainingWrapper wrapper;
+    auto writer = wrapper.beginWrite();
+
+    auto listWriter = writer.beginWrite<ident::aaa>();
+    for (int i = 0; i < count; ++i) {
+      auto s = "a string " + std::to_string(i);
+      listWriter.write(s);
+    }
+    writer.endWrite(std::move(listWriter));
+
+    auto bwriter = writer.beginWrite<ident::bbb>();
+    for (int i = 0; i < count; ++i) {
+      bwriter.write(i);
+    }
+    writer.endWrite(std::move(bwriter));
+
+    writer.write<ident::ccc>(true);
+
+    wrapper.endWrite(std::move(writer));
+
+    buf = std::move(wrapper).serializedData();
+  }
+
+  {
+    ReadRemainingWrapper wrapper = ReadRemainingWrapper(std::move(buf));
+    auto reader = wrapper.beginRead();
+    auto listReader = reader.beginRead<ident::aaa>();
+    listReader.remaining();
+    reader.endRead(std::move(listReader));
+    auto breader = reader.beginRead<ident::bbb>();
+    reader.endRead(std::move(breader));
+    wrapper.endRead(std::move(reader));
+  }
+}
+
+TEST(CursorBasedSerializer, CursorReadRemainingEndOne) {
+  doCursorReadRemainEndTest(1);
+}
+
+TEST(CursorBasedSerializer, CursorReadRemainingEndMany) {
+  doCursorReadRemainEndTest(10);
+}

third-party/thrift/src/thrift/lib/cpp2/protocol/test/cursor.thrift

@@ -126,3 +126,12 @@ typedef Empty EmptyWrapper
 service Example {
   EmptyWrapper identity(1: EmptyWrapper empty);
 }
+
+struct ReadRemaining {
+  1: list<string> aaa;
+  2: list<i32> bbb;
+  3: bool ccc;
+}
+
+@cpp.UseCursorSerialization
+typedef ReadRemaining ReadRemainingWrapper