Fix potential crash bug with BidirectionalStreamQuicImpl::SendRequestHeaders

net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/chromium/bidirectional_stream_quic_impl.h"
 
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/memory/ptr_util.h"
@@ skipping 155 matching lines @@
     stream_ = base::MakeUnique<BidirectionalStreamQuicImpl>(std::move(session));
     stream_->Start(request_info, net_log, send_request_headers_automatically_,
                    this, nullptr);
   }
 
   void SendRequestHeaders() { stream_->SendRequestHeaders(); }
 
   void SendData(const scoped_refptr<IOBuffer>& data,
                 int length,
                 bool end_of_stream) {
-    not_expect_callback_ = true;
     stream_->SendData(data, length, end_of_stream);
-    not_expect_callback_ = false;
   }
 
   void SendvData(const std::vector<scoped_refptr<IOBuffer>>& data,
                  const std::vector<int>& lengths,
                  bool end_of_stream) {
-    not_expect_callback_ = true;
     stream_->SendvData(data, lengths, end_of_stream);
-    not_expect_callback_ = false;

[xunjieli, 2017/06/01 14:22:43]

Should we put back these guard checks? 

[Ryan Hamilton, 2017/06/01 17:10:14]

Done. Of course, putting the back meant that I needed to transform synchronous
write errors into asynchronous delegate notification. I think this ended up
looking reasonable, but it it's a non-trivial change to the CL (and changes a
few test expectations), so PTAL.

   }
 
   // Waits until next Delegate callback.
   void WaitUntilNextCallback(DelegateMethod method) {
     ASSERT_FALSE(on_failed_called_);
     bool is_ready = is_ready_;
     bool headers_received = !response_headers_.empty();
     bool trailers_received = trailers_received_;
     int on_data_read_count = on_data_read_count_;
     int on_data_sent_count = on_data_sent_count_;
@@ skipping 1044 matching lines @@
       static_cast<int64_t>(spdy_request_headers_frame_length + strlen(kBody1) +
                            strlen(kBody2) + strlen(kBody3) + strlen(kBody4) +
                            strlen(kBody5)),
       delegate->GetTotalSentBytes());
   EXPECT_EQ(
       static_cast<int64_t>(spdy_response_headers_frame_length +
                            strlen(kResponseBody) + spdy_trailers_frame_length),
       delegate->GetTotalReceivedBytes());
 }
 
+// Tests that when request headers are delayed and SendData triggers the
+// headers to be sent, if that write fails the stream does not crash.
+TEST_P(BidirectionalStreamQuicImplTest,
+       SendDataWriteErrorCoalesceDataBufferAndHeaderFrame) {
+  QuicStreamOffset header_stream_offset = 0;
+  AddWrite(ConstructInitialSettingsPacket(1, &header_stream_offset));
+  AddWriteError(SYNCHRONOUS, ERR_CONNECTION_REFUSED);
+
+  Initialize();
+
+  BidirectionalStreamRequestInfo request;
+  request.method = "POST";
+  request.url = GURL("http://www.google.com/");
+  request.end_stream_on_headers = false;
+  request.priority = DEFAULT_PRIORITY;
+  request.extra_headers.SetHeader("cookie", std::string(2048, 'A'));
+
+  scoped_refptr<IOBuffer> read_buffer(new IOBuffer(kReadBufferSize));
+  std::unique_ptr<DeleteStreamDelegate> delegate(new DeleteStreamDelegate(
+      read_buffer.get(), kReadBufferSize, DeleteStreamDelegate::ON_FAILED));
+  delegate->DoNotSendRequestHeadersAutomatically();
+  delegate->Start(&request, net_log().bound(), session()->CreateHandle());
+  ConfirmHandshake();
+  delegate->WaitUntilNextCallback(kOnStreamReady);
+
+  // Attempt to send the headers and data.
+  const char kBody1[] = "here are some data";
+  scoped_refptr<StringIOBuffer> buf1(new StringIOBuffer(kBody1));
+  delegate->SendData(buf1, buf1->size(), !kFin);
+
+  EXPECT_TRUE(delegate->on_failed_called());
+}
+
+// Tests that when request headers are delayed and SendvData triggers the
+// headers to be sent, if that write fails the stream does not crash.
+TEST_P(BidirectionalStreamQuicImplTest,
+       SendvDataWriteErrorCoalesceDataBufferAndHeaderFrame) {
+  QuicStreamOffset header_stream_offset = 0;
+  AddWrite(ConstructInitialSettingsPacket(1, &header_stream_offset));
+  AddWriteError(SYNCHRONOUS, ERR_CONNECTION_REFUSED);
+
+  Initialize();
+
+  BidirectionalStreamRequestInfo request;
+  request.method = "POST";
+  request.url = GURL("http://www.google.com/");
+  request.end_stream_on_headers = false;
+  request.priority = DEFAULT_PRIORITY;
+  request.extra_headers.SetHeader("cookie", std::string(2048, 'A'));
+
+  scoped_refptr<IOBuffer> read_buffer(new IOBuffer(kReadBufferSize));
+  std::unique_ptr<DeleteStreamDelegate> delegate(new DeleteStreamDelegate(
+      read_buffer.get(), kReadBufferSize, DeleteStreamDelegate::ON_FAILED));
+  delegate->DoNotSendRequestHeadersAutomatically();
+  delegate->Start(&request, net_log().bound(), session()->CreateHandle());
+  ConfirmHandshake();
+  delegate->WaitUntilNextCallback(kOnStreamReady);
+
+  // Attempt to send the headers and data.
+  const char kBody1[] = "here are some data";
+  const char kBody2[] = "data keep coming";
+  scoped_refptr<StringIOBuffer> buf1(new StringIOBuffer(kBody1));
+  scoped_refptr<StringIOBuffer> buf2(new StringIOBuffer(kBody2));
+  std::vector<int> lengths = {buf1->size(), buf2->size()};
+  delegate->SendvData({buf1, buf2}, lengths, !kFin);
+
+  EXPECT_TRUE(delegate->on_failed_called());
+}
+
 TEST_P(BidirectionalStreamQuicImplTest, PostRequest) {
   SetRequest("POST", "/", DEFAULT_PRIORITY);
   size_t spdy_request_headers_frame_length;
   QuicStreamOffset header_stream_offset = 0;
   AddWrite(ConstructInitialSettingsPacket(1, &header_stream_offset));
   AddWrite(ConstructRequestHeadersPacketInner(
       2, GetNthClientInitiatedStreamId(0), !kFin, DEFAULT_PRIORITY,
       &spdy_request_headers_frame_length, &header_stream_offset));
   AddWrite(ConstructDataPacket(3, kIncludeVersion, kFin, 0, kUploadData,
                                &client_maker_));
@@ skipping 782 matching lines @@
 
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(1, delegate->on_data_read_count());
   EXPECT_EQ(0, delegate->on_data_sent_count());
 }
 
 }  // namespace test
 
 }  // namespace net