trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From masa...@apache.org
Subject [trafficserver] 02/02: Fix heap-use-after-free in QUICStreamFrame::store
Date Thu, 12 Oct 2017 07:42:59 GMT
This is an automated email from the ASF dual-hosted git repository.

masaori pushed a commit to branch quic-latest
in repository https://gitbox.apache.org/repos/asf/trafficserver.git

commit 2250a414d8818b5f6b781a9ee9811521d6022e20
Author: Masaori Koshiba <masaori@apache.org>
AuthorDate: Thu Oct 12 16:34:14 2017 +0900

    Fix heap-use-after-free in QUICStreamFrame::store
    
    Make data of QUICStramFrame ats_unique_buf. Copy data of _write_vio
    to the buffer, when QUICStream sends frame. Ideally this malloc and
    copy should be avoided.
---
 iocore/net/quic/QUICFrame.cc                     |  11 +-
 iocore/net/quic/QUICFrame.h                      |   4 +-
 iocore/net/quic/test/test_QUICFrame.cc           |  50 +++++-
 iocore/net/quic/test/test_QUICFrameDispatcher.cc |   7 +-
 iocore/net/quic/test/test_QUICStream.cc          | 215 +++++++++++++----------
 5 files changed, 174 insertions(+), 113 deletions(-)

diff --git a/iocore/net/quic/QUICFrame.cc b/iocore/net/quic/QUICFrame.cc
index 69951d4..4bb02b4 100644
--- a/iocore/net/quic/QUICFrame.cc
+++ b/iocore/net/quic/QUICFrame.cc
@@ -72,9 +72,9 @@ QUICFrame::reset(const uint8_t *buf, size_t len)
 // STREAM Frame
 //
 
-QUICStreamFrame::QUICStreamFrame(const uint8_t *data, size_t data_len, QUICStreamId stream_id,
QUICOffset offset, bool last)
+QUICStreamFrame::QUICStreamFrame(ats_unique_buf data, size_t data_len, QUICStreamId stream_id,
QUICOffset offset, bool last)
 {
-  this->_data      = data;
+  this->_data      = std::move(data);
   this->_data_len  = data_len;
   this->_stream_id = stream_id;
   this->_offset    = offset;
@@ -183,7 +183,7 @@ QUICStreamFrame::data() const
   if (this->_buf) {
     return this->_buf + this->_get_data_offset();
   } else {
-    return this->_data;
+    return this->_data.get();
   }
 }
 
@@ -1315,8 +1315,11 @@ QUICFrameFactory::fast_create(const uint8_t *buf, size_t len)
 QUICStreamFrameUPtr
 QUICFrameFactory::create_stream_frame(const uint8_t *data, size_t data_len, QUICStreamId
stream_id, QUICOffset offset, bool last)
 {
+  ats_unique_buf buf = ats_unique_malloc(data_len);
+  memcpy(buf.get(), data, data_len);
+
   QUICStreamFrame *frame = quicStreamFrameAllocator.alloc();
-  new (frame) QUICStreamFrame(data, data_len, stream_id, offset, last);
+  new (frame) QUICStreamFrame(std::move(buf), data_len, stream_id, offset, last);
   return QUICStreamFrameUPtr(frame, &QUICFrameDeleter::delete_stream_frame);
 }
 
diff --git a/iocore/net/quic/QUICFrame.h b/iocore/net/quic/QUICFrame.h
index 772d65a..9f2b356 100644
--- a/iocore/net/quic/QUICFrame.h
+++ b/iocore/net/quic/QUICFrame.h
@@ -59,7 +59,7 @@ class QUICStreamFrame : public QUICFrame
 public:
   QUICStreamFrame() : QUICFrame() {}
   QUICStreamFrame(const uint8_t *buf, size_t len) : QUICFrame(buf, len) {}
-  QUICStreamFrame(const uint8_t *buf, size_t len, QUICStreamId streamid, QUICOffset offset,
bool last = false);
+  QUICStreamFrame(ats_unique_buf buf, size_t len, QUICStreamId streamid, QUICOffset offset,
bool last = false);
   virtual QUICFrameType type() const override;
   virtual size_t size() const override;
   virtual void store(uint8_t *buf, size_t *len) const override;
@@ -74,7 +74,7 @@ public:
   LINK(QUICStreamFrame, link);
 
 private:
-  const uint8_t *_data    = nullptr;
+  ats_unique_buf _data    = {nullptr, [](void *p) { ats_free(p); }};
   size_t _data_len        = 0;
   QUICStreamId _stream_id = 0;
   QUICOffset _offset      = 0;
diff --git a/iocore/net/quic/test/test_QUICFrame.cc b/iocore/net/quic/test/test_QUICFrame.cc
index 90c82fb..36f1eee 100644
--- a/iocore/net/quic/test/test_QUICFrame.cc
+++ b/iocore/net/quic/test/test_QUICFrame.cc
@@ -55,11 +55,14 @@ TEST_CASE("QUICFrame Type", "[quic]")
 
 TEST_CASE("Construct QUICFrame", "[quic]")
 {
-  uint8_t payload[] = "foo";
+  uint8_t raw[]          = "foo";
+  ats_unique_buf payload = ats_unique_malloc(sizeof(raw));
+  memcpy(payload.get(), raw, sizeof(raw));
+
   uint8_t buf[65536];
   size_t len;
 
-  QUICStreamFrame frame1(payload, sizeof(payload), 0xffcc9966, 0xffddbb9977553311);
+  QUICStreamFrame frame1(std::move(payload), sizeof(raw), 0xffcc9966, 0xffddbb9977553311);
   frame1.store(buf, &len);
   CHECK(frame1.type() == QUICFrameType::STREAM);
   CHECK(frame1.size() == 19);
@@ -119,7 +122,12 @@ TEST_CASE("Store STREAM Frame", "[quic]")
     0x00, 0x05,                   // Data Length
     0x01, 0x02, 0x03, 0x04, 0x05, // Stream Data
   };
-  QUICStreamFrame streamFrame1(reinterpret_cast<const uint8_t *>("\x01\x02\x03\x04\x05"),
5, 0x01, 0x00);
+
+  uint8_t raw1[]          = "\x01\x02\x03\x04\x05";
+  ats_unique_buf payload1 = ats_unique_malloc(5);
+  memcpy(payload1.get(), raw1, 5);
+
+  QUICStreamFrame streamFrame1(std::move(payload1), 5, 0x01, 0x00);
   streamFrame1.store(buf, &len);
   CHECK(len == 9);
   CHECK(memcmp(buf, expected1, len) == 0);
@@ -132,7 +140,11 @@ TEST_CASE("Store STREAM Frame", "[quic]")
     0x00, 0x05,                   // Data Length
     0x01, 0x02, 0x03, 0x04, 0x05, // Stream Data
   };
-  QUICStreamFrame streamFrame2(reinterpret_cast<const uint8_t *>("\x01\x02\x03\x04\x05"),
5, 0x01, 0x01);
+  uint8_t raw2[]          = "\x01\x02\x03\x04\x05";
+  ats_unique_buf payload2 = ats_unique_malloc(5);
+  memcpy(payload2.get(), raw2, 5);
+
+  QUICStreamFrame streamFrame2(std::move(payload2), 5, 0x01, 0x01);
   streamFrame2.store(buf, &len);
   CHECK(len == 11);
   CHECK(memcmp(buf, expected2, len) == 0);
@@ -145,7 +157,11 @@ TEST_CASE("Store STREAM Frame", "[quic]")
     0x00, 0x05,                   // Data Length
     0x01, 0x02, 0x03, 0x04, 0x05, // Stream Data
   };
-  QUICStreamFrame streamFrame3(reinterpret_cast<const uint8_t *>("\x01\x02\x03\x04\x05"),
5, 0x01, 0x010000);
+  uint8_t raw3[]          = "\x01\x02\x03\x04\x05";
+  ats_unique_buf payload3 = ats_unique_malloc(5);
+  memcpy(payload3.get(), raw3, 5);
+
+  QUICStreamFrame streamFrame3(std::move(payload3), 5, 0x01, 0x010000);
   streamFrame3.store(buf, &len);
   CHECK(len == 13);
   CHECK(memcmp(buf, expected3, len) == 0);
@@ -158,7 +174,11 @@ TEST_CASE("Store STREAM Frame", "[quic]")
     0x00, 0x05,                                     // Data Length
     0x01, 0x02, 0x03, 0x04, 0x05,                   // Stream Data
   };
-  QUICStreamFrame streamFrame4(reinterpret_cast<const uint8_t *>("\x01\x02\x03\x04\x05"),
5, 0x01, 0x0100000000);
+  uint8_t raw4[]          = "\x01\x02\x03\x04\x05";
+  ats_unique_buf payload4 = ats_unique_malloc(5);
+  memcpy(payload4.get(), raw4, 5);
+
+  QUICStreamFrame streamFrame4(std::move(payload4), 5, 0x01, 0x0100000000);
   streamFrame4.store(buf, &len);
   CHECK(len == 17);
   CHECK(memcmp(buf, expected4, len) == 0);
@@ -171,7 +191,11 @@ TEST_CASE("Store STREAM Frame", "[quic]")
     0x00, 0x05,                                     // Data Length
     0x01, 0x02, 0x03, 0x04, 0x05,                   // Stream Data
   };
-  QUICStreamFrame streamFrame5(reinterpret_cast<const uint8_t *>("\x01\x02\x03\x04\x05"),
5, 0x0100, 0x0100000000);
+  uint8_t raw5[]          = "\x01\x02\x03\x04\x05";
+  ats_unique_buf payload5 = ats_unique_malloc(5);
+  memcpy(payload5.get(), raw5, 5);
+
+  QUICStreamFrame streamFrame5(std::move(payload5), 5, 0x0100, 0x0100000000);
   streamFrame5.store(buf, &len);
   CHECK(len == 18);
   CHECK(memcmp(buf, expected5, len) == 0);
@@ -184,7 +208,11 @@ TEST_CASE("Store STREAM Frame", "[quic]")
     0x00, 0x05,                                     // Data Length
     0x01, 0x02, 0x03, 0x04, 0x05,                   // Stream Data
   };
-  QUICStreamFrame streamFrame6(reinterpret_cast<const uint8_t *>("\x01\x02\x03\x04\x05"),
5, 0x010000, 0x0100000000);
+  uint8_t raw6[]          = "\x01\x02\x03\x04\x05";
+  ats_unique_buf payload6 = ats_unique_malloc(5);
+  memcpy(payload6.get(), raw6, 5);
+
+  QUICStreamFrame streamFrame6(std::move(payload6), 5, 0x010000, 0x0100000000);
   streamFrame6.store(buf, &len);
   CHECK(len == 19);
   CHECK(memcmp(buf, expected6, len) == 0);
@@ -197,7 +225,11 @@ TEST_CASE("Store STREAM Frame", "[quic]")
     0x00, 0x05,                                     // Data Length
     0x01, 0x02, 0x03, 0x04, 0x05,                   // Stream Data
   };
-  QUICStreamFrame streamFrame7(reinterpret_cast<const uint8_t *>("\x01\x02\x03\x04\x05"),
5, 0x01000000, 0x0100000000);
+  uint8_t raw7[]          = "\x01\x02\x03\x04\x05";
+  ats_unique_buf payload7 = ats_unique_malloc(5);
+  memcpy(payload7.get(), raw7, 5);
+
+  QUICStreamFrame streamFrame7(std::move(payload7), 5, 0x01000000, 0x0100000000);
   streamFrame7.store(buf, &len);
   CHECK(len == 20);
   CHECK(memcmp(buf, expected7, len) == 0);
diff --git a/iocore/net/quic/test/test_QUICFrameDispatcher.cc b/iocore/net/quic/test/test_QUICFrameDispatcher.cc
index 8573dce..215303f 100644
--- a/iocore/net/quic/test/test_QUICFrameDispatcher.cc
+++ b/iocore/net/quic/test/test_QUICFrameDispatcher.cc
@@ -29,8 +29,11 @@
 
 TEST_CASE("QUICFrameHandler", "[quic]")
 {
-  uint8_t payload[] = {0x01};
-  QUICStreamFrame streamFrame(payload, 1, 0x03, 0);
+  uint8_t raw[]          = {0x01};
+  ats_unique_buf payload = ats_unique_malloc(1);
+  memcpy(payload.get(), raw, 1);
+
+  QUICStreamFrame streamFrame(std::move(payload), 1, 0x03, 0);
 
   auto connection           = new MockQUICConnection();
   auto streamManager        = new MockQUICStreamManager();
diff --git a/iocore/net/quic/test/test_QUICStream.cc b/iocore/net/quic/test/test_QUICStream.cc
index 632ad91..ef523ab 100644
--- a/iocore/net/quic/test/test_QUICStream.cc
+++ b/iocore/net/quic/test/test_QUICStream.cc
@@ -26,101 +26,124 @@
 #include "quic/QUICStream.h"
 #include "quic/Mock.h"
 
-namespace
+TEST_CASE("QUICStream", "[quic]")
 {
-// Test Data
-uint8_t payload[]  = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c,
0x0d, 0x0e, 0x0f, 0x10};
-uint32_t stream_id = 0x03;
-
-std::shared_ptr<QUICStreamFrame> frame_1 = std::make_shared<QUICStreamFrame>(payload,
2, stream_id, 0);
-std::shared_ptr<QUICStreamFrame> frame_2 = std::make_shared<QUICStreamFrame>(payload
+ 2, 2, stream_id, 2);
-std::shared_ptr<QUICStreamFrame> frame_3 = std::make_shared<QUICStreamFrame>(payload
+ 4, 2, stream_id, 4);
-std::shared_ptr<QUICStreamFrame> frame_4 = std::make_shared<QUICStreamFrame>(payload
+ 6, 2, stream_id, 6);
-std::shared_ptr<QUICStreamFrame> frame_5 = std::make_shared<QUICStreamFrame>(payload
+ 8, 2, stream_id, 8);
-std::shared_ptr<QUICStreamFrame> frame_6 = std::make_shared<QUICStreamFrame>(payload
+ 10, 2, stream_id, 10);
-std::shared_ptr<QUICStreamFrame> frame_7 = std::make_shared<QUICStreamFrame>(payload
+ 12, 2, stream_id, 12);
-std::shared_ptr<QUICStreamFrame> frame_8 = std::make_shared<QUICStreamFrame>(payload
+ 14, 2, stream_id, 14);
-
-TEST_CASE("QUICStream_assembling_byte_stream_1", "[quic]")
-{
-  MIOBuffer *read_buffer = new_MIOBuffer(BUFFER_SIZE_INDEX_4K);
-  IOBufferReader *reader = read_buffer->alloc_reader();
-  MockQUICFrameTransmitter tx;
-
-  std::unique_ptr<QUICStream> stream(new QUICStream());
-  stream->init(&tx, 0, stream_id, 1024, 1024);
-  stream->do_io_read(nullptr, 0, read_buffer);
-
-  stream->recv(frame_1);
-  stream->recv(frame_2);
-  stream->recv(frame_3);
-  stream->recv(frame_4);
-  stream->recv(frame_5);
-  stream->recv(frame_6);
-  stream->recv(frame_7);
-  stream->recv(frame_8);
-
-  uint8_t buf[32];
-  int64_t len = reader->read_avail();
-  reader->read(buf, len);
-
-  CHECK(len == 16);
-  CHECK(memcmp(buf, payload, len) == 0);
-}
-
-TEST_CASE("QUICStream_assembling_byte_stream_2", "[quic]")
-{
-  MIOBuffer *read_buffer = new_MIOBuffer(BUFFER_SIZE_INDEX_4K);
-  IOBufferReader *reader = read_buffer->alloc_reader();
-  MockQUICFrameTransmitter tx;
-
-  std::unique_ptr<QUICStream> stream(new QUICStream());
-  stream->init(&tx, 0, stream_id);
-  stream->do_io_read(nullptr, 0, read_buffer);
-
-  stream->recv(frame_8);
-  stream->recv(frame_7);
-  stream->recv(frame_6);
-  stream->recv(frame_5);
-  stream->recv(frame_4);
-  stream->recv(frame_3);
-  stream->recv(frame_2);
-  stream->recv(frame_1);
-
-  uint8_t buf[32];
-  int64_t len = reader->read_avail();
-  reader->read(buf, len);
-
-  CHECK(len == 16);
-  CHECK(memcmp(buf, payload, len) == 0);
-}
-
-TEST_CASE("QUICStream_assembling_byte_stream_3", "[quic]")
-{
-  MIOBuffer *read_buffer = new_MIOBuffer(BUFFER_SIZE_INDEX_4K);
-  IOBufferReader *reader = read_buffer->alloc_reader();
-  MockQUICFrameTransmitter tx;
-
-  std::unique_ptr<QUICStream> stream(new QUICStream());
-  stream->init(&tx, 0, stream_id);
-  stream->do_io_read(nullptr, 0, read_buffer);
-
-  stream->recv(frame_8);
-  stream->recv(frame_7);
-  stream->recv(frame_6);
-  stream->recv(frame_7); // duplicated frame
-  stream->recv(frame_5);
-  stream->recv(frame_3);
-  stream->recv(frame_1);
-  stream->recv(frame_2);
-  stream->recv(frame_4);
-  stream->recv(frame_5); // duplicated frame
-
-  uint8_t buf[32];
-  int64_t len = reader->read_avail();
-  reader->read(buf, len);
-
-  CHECK(len == 16);
-  CHECK(memcmp(buf, payload, len) == 0);
-}
+  // Test Data
+  uint8_t payload[]  = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b,
0x0c, 0x0d, 0x0e, 0x0f, 0x10};
+  uint32_t stream_id = 0x03;
+
+  ats_unique_buf payload1 = ats_unique_malloc(2);
+  memcpy(payload1.get(), payload, 2);
+  std::shared_ptr<QUICStreamFrame> frame_1 = std::make_shared<QUICStreamFrame>(std::move(payload1),
2, stream_id, 0);
+
+  ats_unique_buf payload2 = ats_unique_malloc(2);
+  memcpy(payload2.get(), payload + 2, 2);
+  std::shared_ptr<QUICStreamFrame> frame_2 = std::make_shared<QUICStreamFrame>(std::move(payload2),
2, stream_id, 2);
+
+  ats_unique_buf payload3 = ats_unique_malloc(2);
+  memcpy(payload3.get(), payload + 4, 2);
+  std::shared_ptr<QUICStreamFrame> frame_3 = std::make_shared<QUICStreamFrame>(std::move(payload3),
2, stream_id, 4);
+
+  ats_unique_buf payload4 = ats_unique_malloc(2);
+  memcpy(payload4.get(), payload + 6, 2);
+  std::shared_ptr<QUICStreamFrame> frame_4 = std::make_shared<QUICStreamFrame>(std::move(payload4),
2, stream_id, 6);
+
+  ats_unique_buf payload5 = ats_unique_malloc(2);
+  memcpy(payload5.get(), payload + 8, 2);
+  std::shared_ptr<QUICStreamFrame> frame_5 = std::make_shared<QUICStreamFrame>(std::move(payload5),
2, stream_id, 8);
+
+  ats_unique_buf payload6 = ats_unique_malloc(2);
+  memcpy(payload6.get(), payload + 10, 2);
+  std::shared_ptr<QUICStreamFrame> frame_6 = std::make_shared<QUICStreamFrame>(std::move(payload6),
2, stream_id, 10);
+
+  ats_unique_buf payload7 = ats_unique_malloc(2);
+  memcpy(payload7.get(), payload + 12, 2);
+  std::shared_ptr<QUICStreamFrame> frame_7 = std::make_shared<QUICStreamFrame>(std::move(payload7),
2, stream_id, 12);
+
+  ats_unique_buf payload8 = ats_unique_malloc(2);
+  memcpy(payload8.get(), payload + 14, 2);
+  std::shared_ptr<QUICStreamFrame> frame_8 = std::make_shared<QUICStreamFrame>(std::move(payload8),
2, stream_id, 14);
+
+  SECTION("QUICStream_assembling_byte_stream_1")
+  {
+    MIOBuffer *read_buffer = new_MIOBuffer(BUFFER_SIZE_INDEX_4K);
+    IOBufferReader *reader = read_buffer->alloc_reader();
+    MockQUICFrameTransmitter tx;
+
+    std::unique_ptr<QUICStream> stream(new QUICStream());
+    stream->init(&tx, 0, stream_id, 1024, 1024);
+    stream->do_io_read(nullptr, 0, read_buffer);
+
+    stream->recv(frame_1);
+    stream->recv(frame_2);
+    stream->recv(frame_3);
+    stream->recv(frame_4);
+    stream->recv(frame_5);
+    stream->recv(frame_6);
+    stream->recv(frame_7);
+    stream->recv(frame_8);
+
+    uint8_t buf[32];
+    int64_t len = reader->read_avail();
+    reader->read(buf, len);
+
+    CHECK(len == 16);
+    CHECK(memcmp(buf, payload, len) == 0);
+  }
+
+  SECTION("QUICStream_assembling_byte_stream_2")
+  {
+    MIOBuffer *read_buffer = new_MIOBuffer(BUFFER_SIZE_INDEX_4K);
+    IOBufferReader *reader = read_buffer->alloc_reader();
+    MockQUICFrameTransmitter tx;
+
+    std::unique_ptr<QUICStream> stream(new QUICStream());
+    stream->init(&tx, 0, stream_id);
+    stream->do_io_read(nullptr, 0, read_buffer);
+
+    stream->recv(frame_8);
+    stream->recv(frame_7);
+    stream->recv(frame_6);
+    stream->recv(frame_5);
+    stream->recv(frame_4);
+    stream->recv(frame_3);
+    stream->recv(frame_2);
+    stream->recv(frame_1);
+
+    uint8_t buf[32];
+    int64_t len = reader->read_avail();
+    reader->read(buf, len);
+
+    CHECK(len == 16);
+    CHECK(memcmp(buf, payload, len) == 0);
+  }
+
+  SECTION("QUICStream_assembling_byte_stream_3")
+  {
+    MIOBuffer *read_buffer = new_MIOBuffer(BUFFER_SIZE_INDEX_4K);
+    IOBufferReader *reader = read_buffer->alloc_reader();
+    MockQUICFrameTransmitter tx;
+
+    std::unique_ptr<QUICStream> stream(new QUICStream());
+    stream->init(&tx, 0, stream_id);
+    stream->do_io_read(nullptr, 0, read_buffer);
+
+    stream->recv(frame_8);
+    stream->recv(frame_7);
+    stream->recv(frame_6);
+    stream->recv(frame_7); // duplicated frame
+    stream->recv(frame_5);
+    stream->recv(frame_3);
+    stream->recv(frame_1);
+    stream->recv(frame_2);
+    stream->recv(frame_4);
+    stream->recv(frame_5); // duplicated frame
+
+    uint8_t buf[32];
+    int64_t len = reader->read_avail();
+    reader->read(buf, len);
+
+    CHECK(len == 16);
+    CHECK(memcmp(buf, payload, len) == 0);
+  }
 }

-- 
To stop receiving notification emails like this one, please contact
"commits@trafficserver.apache.org" <commits@trafficserver.apache.org>.

Mime
View raw message