Add CHECK's to debug QuicStreamSequencerBuffer in weird state. Add a...

Add CHECK's to debug QuicStreamSequencerBuffer in weird state. Add a destruction indicator to detect free after use.

Also remove some tests because CHECK's prevent some corner case from
happening.

R=rch@chromium.org
BUG=664200

Review-Url: https://codereview.chromium.org/2519333006
Cr-Commit-Position: refs/heads/master@{#434659}

net/quic/core/quic_stream_sequencer_buffer.cc

@@ -50,12 +50,17 @@ QuicStreamSequencerBuffer::QuicStreamSequencerBuffer(size_t max_capacity_bytes)
       blocks_count_(
           ceil(static_cast<double>(max_capacity_bytes) / kBlockSizeBytes)),
       total_bytes_read_(0),
-      blocks_(nullptr) {
+      blocks_(nullptr),
+      destruction_indicator_(123456) {
+  CHECK_GT(blocks_count_, 1u)
+      << "blocks_count_ = " << blocks_count_
+      << ", max_buffer_capacity_bytes_ = " << max_buffer_capacity_bytes_;
   Clear();
 }
 
 QuicStreamSequencerBuffer::~QuicStreamSequencerBuffer() {
   Clear();
+  destruction_indicator_ = 654321;
 }
 
 void QuicStreamSequencerBuffer::Clear() {
@@ -92,6 +97,7 @@ QuicErrorCode QuicStreamSequencerBuffer::OnStreamData(
     QuicTime timestamp,
     size_t* const bytes_buffered,
     std::string* error_details) {
+  CHECK_EQ(destruction_indicator_, 123456) << "This object has been destructed";
   *bytes_buffered = 0;
   QuicStreamOffset offset = starting_offset;
   size_t size = data.size();
@@ -273,9 +279,12 @@ QuicErrorCode QuicStreamSequencerBuffer::Readv(const iovec* dest_iov,
                                                size_t dest_count,
                                                size_t* bytes_read,
                                                string* error_details) {
+  CHECK_EQ(destruction_indicator_, 123456) << "This object has been destructed";
+
   *bytes_read = 0;
   for (size_t i = 0; i < dest_count && ReadableBytes() > 0; ++i) {
     char* dest = reinterpret_cast<char*>(dest_iov[i].iov_base);
+    CHECK_NE(dest, nullptr);
     size_t dest_remaining = dest_iov[i].iov_len;
     while (dest_remaining > 0 && ReadableBytes() > 0) {
       size_t block_idx = NextBlockToRead();
@@ -329,6 +338,8 @@ QuicErrorCode QuicStreamSequencerBuffer::Readv(const iovec* dest_iov,
 
 int QuicStreamSequencerBuffer::GetReadableRegions(struct iovec* iov,
                                                   int iov_count) const {
+  CHECK_EQ(destruction_indicator_, 123456) << "This object has been destructed";
+
   DCHECK(iov != nullptr);
   DCHECK_GT(iov_count, 0);
 
@@ -387,6 +398,8 @@ int QuicStreamSequencerBuffer::GetReadableRegions(struct iovec* iov,
 
 bool QuicStreamSequencerBuffer::GetReadableRegion(iovec* iov,
                                                   QuicTime* timestamp) const {
+  CHECK_EQ(destruction_indicator_, 123456) << "This object has been destructed";
+
   if (ReadableBytes() == 0) {
     iov[0].iov_base = nullptr;
     iov[0].iov_len = 0;
@@ -425,6 +438,8 @@ bool QuicStreamSequencerBuffer::GetReadableRegion(iovec* iov,
 }
 
 bool QuicStreamSequencerBuffer::MarkConsumed(size_t bytes_used) {
+  CHECK_EQ(destruction_indicator_, 123456) << "This object has been destructed";
+
   if (bytes_used > ReadableBytes()) {
     return false;
   }

net/quic/core/quic_stream_sequencer_buffer.h

@@ -248,6 +248,11 @@ class NET_EXPORT_PRIVATE QuicStreamSequencerBuffer {
   // Stores all the buffered frames' start offset, length and arrival time.
   std::map<QuicStreamOffset, FrameInfo> frame_arrival_time_map_;
 
+  // For debugging use after free, assigned to 123456 in constructor and 654321
+  // in destructor. As long as it's not 123456, this means either use after free
+  // or memory corruption.
+  int32_t destruction_indicator_;
+
   DISALLOW_COPY_AND_ASSIGN(QuicStreamSequencerBuffer);
 };
 }  // namespace net

net/quic/core/quic_stream_sequencer_buffer_test.cc

@@ -332,26 +332,6 @@ TEST_F(QuicStreamSequencerBufferTest, Readv100Bytes) {
   EXPECT_TRUE(helper_->CheckBufferInvariants());
 }
 
-TEST_F(QuicStreamSequencerBufferTest, ReadvError) {
-  string source = string(100, 'b');
-  clock_.AdvanceTime(QuicTime::Delta::FromSeconds(1));
-  QuicTime t1 = clock_.ApproximateNow();
-  // Write something into [0, 100).
-  size_t written;
-  buffer_->OnStreamData(0, source, t1, &written, &error_details_);
-  EXPECT_TRUE(helper_->GetBlock(0) != nullptr);
-  EXPECT_TRUE(buffer_->HasBytesToRead());
-  // Read into a iovec array with total capacity of 120 bytes.
-  iovec iov{nullptr, 120};
-  size_t read;
-  EXPECT_EQ(QUIC_STREAM_SEQUENCER_INVALID_STATE,
-            buffer_->Readv(&iov, 1, &read, &error_details_));
-  EXPECT_EQ(0u,
-            error_details_.find(
-                "QuicStreamSequencerBuffer error: Readv() dest == nullptr: true"
-                " blocks_[0] == nullptr: false"));
-}
-
 TEST_F(QuicStreamSequencerBufferTest, ReadvAcrossBlocks) {
   string source(kBlockSizeBytes + 50, 'a');
   // Write 1st block to full and extand 50 bytes to next block.

net/quic/core/quic_stream_sequencer_test.cc

@@ -659,31 +659,16 @@ TEST_F(QuicStreamSequencerTest, OutOfOrderTimestamps) {
   EXPECT_EQ(0u, sequencer_->NumBytesBuffered());
 }
 
-// TODO(danzh): Figure out the way to implement this test case without the use
-// of unsupported StringPiece constructor.
-#if 0
 TEST_F(QuicStreamSequencerTest, OnStreamFrameWithNullSource) {
   // Pass in a frame with data pointing to null address, expect to close
   // connection with error.
-  StringPiece source(nullptr, 5u);
+  StringPiece source;
+  source.set(nullptr, 5u);
   QuicStreamFrame frame(kClientDataStreamId1, false, 1, source);
   EXPECT_CALL(stream_, CloseConnectionWithDetails(
                            QUIC_STREAM_SEQUENCER_INVALID_STATE, _));
   sequencer_->OnStreamFrame(frame);
 }
-#endif
-
-TEST_F(QuicStreamSequencerTest, ReadvError) {
-  EXPECT_CALL(stream_, OnDataAvailable());
-  string source(100, 'a');
-  OnFrame(0u, source.data());
-  EXPECT_EQ(source.length(), sequencer_->NumBytesBuffered());
-  // Pass in a null iovec, expect to tear down connection.
-  EXPECT_CALL(stream_, CloseConnectionWithDetails(
-                           QUIC_STREAM_SEQUENCER_INVALID_STATE, _));
-  iovec iov{nullptr, 512};
-  sequencer_->Readv(&iov, 1u);
-}
 
 }  // namespace
 }  // namespace test