[SPDY] Change semantics of SpdyBuffer::GetIOBufferForRemainingData()

Make its return value be able to outlive the SpdyBuffer.

This should fix the bug below.

BUG=249725
R=rtenneti@chromium.org

Review URL: https://codereview.chromium.org/18512009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@211442 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 63 insertions, 11 deletions

diff --git a/net/spdy/spdy_buffer.cc b/net/spdy/spdy_buffer.cc
index ef3e640..b93ae0a 100644
--- a/net/spdy/spdy_buffer.cc
+++ b/net/spdy/spdy_buffer.cc
@@ -29,15 +29,42 @@ scoped_ptr<SpdyFrame> MakeSpdyFrame(const char* data, size_t size) {
 
 }  // namespace
 
+// This class is an IOBuffer implementation that simply holds a
+// reference to a SharedFrame object and a fixed offset. Used by
+// SpdyBuffer::GetIOBufferForRemainingData().
+class SpdyBuffer::SharedFrameIOBuffer : public IOBuffer {
+ public:
+  SharedFrameIOBuffer(const scoped_refptr<SharedFrame>& shared_frame,
+                      size_t offset)
+      : IOBuffer(shared_frame->data->data() + offset),
+        shared_frame_(shared_frame),
+        offset_(offset) {}
+
+ private:
+  virtual ~SharedFrameIOBuffer() {
+    // Prevent ~IOBuffer() from trying to delete |data_|.
+    data_ = NULL;
+  }
+
+  const scoped_refptr<SharedFrame> shared_frame_;
+  const size_t offset_;
+
+  DISALLOW_COPY_AND_ASSIGN(SharedFrameIOBuffer);
+};
+
 SpdyBuffer::SpdyBuffer(scoped_ptr<SpdyFrame> frame)
-    : frame_(frame.Pass()),
-      offset_(0) {}
+    : shared_frame_(new SharedFrame()),
+      offset_(0) {
+  shared_frame_->data = frame.Pass();
+}
 
 // The given data may not be strictly a SPDY frame; we (ab)use
 // |frame_| just as a container.
 SpdyBuffer::SpdyBuffer(const char* data, size_t size) :
-    frame_(MakeSpdyFrame(data, size)),
-    offset_(0) {}
+    shared_frame_(new SharedFrame()),
+    offset_(0) {
+  shared_frame_->data = MakeSpdyFrame(data, size);
+}
 
 SpdyBuffer::~SpdyBuffer() {
   if (GetRemainingSize() > 0)
@@ -45,11 +72,11 @@ SpdyBuffer::~SpdyBuffer() {
 }
 
 const char* SpdyBuffer::GetRemainingData() const {
-  return frame_->data() + offset_;
+  return shared_frame_->data->data() + offset_;
 }
 
 size_t SpdyBuffer::GetRemainingSize() const {
-  return frame_->size() - offset_;
+  return shared_frame_->data->size() - offset_;
 }
 
 void SpdyBuffer::AddConsumeCallback(const ConsumeCallback& consume_callback) {
@@ -61,7 +88,7 @@ void SpdyBuffer::Consume(size_t consume_size) {
 };
 
 IOBuffer* SpdyBuffer::GetIOBufferForRemainingData() {
-  return new WrappedIOBuffer(GetRemainingData());
+  return new SharedFrameIOBuffer(shared_frame_, offset_);
 }
 
 void SpdyBuffer::ConsumeHelper(size_t consume_size,
diff --git a/net/spdy/spdy_buffer.h b/net/spdy/spdy_buffer.h
index 69d39ea..60bdab7 100644
--- a/net/spdy/spdy_buffer.h
+++ b/net/spdy/spdy_buffer.h
@@ -10,6 +10,7 @@
 
 #include "base/basictypes.h"
 #include "base/callback_forward.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "net/base/net_export.h"
 
@@ -73,15 +74,25 @@ class NET_EXPORT_PRIVATE SpdyBuffer {
   void Consume(size_t consume_size);
 
   // Returns an IOBuffer pointing to the data starting at
-  // GetRemainingData(). Use with care; the returned IOBuffer must not
-  // be used past the lifetime of this object, and it is not updated
-  // when Consume() is called.
+  // GetRemainingData(). Use with care; the returned IOBuffer is not
+  // updated when Consume() is called. However, it may still be used
+  // past the lifetime of this object.
+  //
+  // This is used with Socket::Write(), which takes an IOBuffer* that
+  // may be written to even after the socket itself is destroyed. (See
+  // http://crbug.com/249725 .)
   IOBuffer* GetIOBufferForRemainingData();
 
  private:
   void ConsumeHelper(size_t consume_size, ConsumeSource consume_source);
 
-  const scoped_ptr<SpdyFrame> frame_;
+  // Ref-count the passed-in SpdyFrame to support the semantics of
+  // |GetIOBufferForRemainingData()|.
+  typedef base::RefCountedData<scoped_ptr<SpdyFrame> > SharedFrame;
+
+  class SharedFrameIOBuffer;
+
+  const scoped_refptr<SharedFrame> shared_frame_;
   std::vector<ConsumeCallback> consume_callbacks_;
   size_t offset_;
 
diff --git a/net/spdy/spdy_buffer_unittest.cc b/net/spdy/spdy_buffer_unittest.cc
index 5a8bfc8..7c14ee9 100644
--- a/net/spdy/spdy_buffer_unittest.cc
+++ b/net/spdy/spdy_buffer_unittest.cc
@@ -5,6 +5,7 @@
 #include "net/spdy/spdy_buffer.h"
 
 #include <cstddef>
+#include <cstring>
 #include <string>
 
 #include "base/basictypes.h"
@@ -118,6 +119,19 @@ TEST_F(SpdyBufferTest, GetIOBufferForRemainingData) {
   EXPECT_EQ(expectedData, std::string(io_buffer->data(), io_buffer_size));
 }
 
+// Make sure the IOBuffer returned by GetIOBufferForRemainingData()
+// outlives the buffer itself.
+TEST_F(SpdyBufferTest, IOBufferForRemainingDataOutlivesBuffer) {
+  scoped_ptr<SpdyBuffer> buffer(new SpdyBuffer(kData, kDataSize));
+
+  scoped_refptr<IOBuffer> io_buffer = buffer->GetIOBufferForRemainingData();
+  buffer.reset();
+
+  // This will cause a use-after-free error if |io_buffer| doesn't
+  // outlive |buffer|.
+  std::memcpy(io_buffer->data(), kData, kDataSize);
+}
+
 }  // namespace
 
 }  // namespace net