trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sc...@apache.org
Subject [trafficserver] branch quic-latest updated: QUIC: Mem leaking with QUICPacketHeader
Date Mon, 05 Mar 2018 02:25:35 GMT
This is an automated email from the ASF dual-hosted git repository.

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


The following commit(s) were added to refs/heads/quic-latest by this push:
     new c3af882  QUIC: Mem leaking with QUICPacketHeader
c3af882 is described below

commit c3af882434fd279e82e5534a55f2faded535e957
Author: scw00 <scw00@apache.org>
AuthorDate: Mon Mar 5 08:31:34 2018 +0800

    QUIC: Mem leaking with QUICPacketHeader
---
 iocore/net/quic/QUICPacket.cc | 81 ++++++++++++++++++++++---------------------
 iocore/net/quic/QUICPacket.h  | 78 +++++++++++++++++++++++++++++------------
 2 files changed, 98 insertions(+), 61 deletions(-)

diff --git a/iocore/net/quic/QUICPacket.cc b/iocore/net/quic/QUICPacket.cc
index a14140d..a76d068 100644
--- a/iocore/net/quic/QUICPacket.cc
+++ b/iocore/net/quic/QUICPacket.cc
@@ -61,52 +61,54 @@ QUICPacketHeader::packet_size() const
   return this->_buf_len;
 }
 
-QUICPacketHeader *
+QUICPacketHeaderUPtr
 QUICPacketHeader::load(const uint8_t *buf, size_t len, QUICPacketNumber base)
 {
+  QUICPacketHeaderUPtr header = QUICPacketHeaderUPtr(nullptr, &QUICPacketHeaderDeleter::delete_null_header);
   if (QUICTypeUtil::has_long_header(buf)) {
     QUICPacketLongHeader *long_header = quicPacketLongHeaderAllocator.alloc();
     new (long_header) QUICPacketLongHeader(buf, len, base);
-    return long_header;
+    header = QUICPacketHeaderUPtr(long_header, &QUICPacketHeaderDeleter::delete_long_header);
   } else {
     QUICPacketShortHeader *short_header = quicPacketShortHeaderAllocator.alloc();
     new (short_header) QUICPacketShortHeader(buf, len, base);
-    return short_header;
+    header = QUICPacketHeaderUPtr(short_header, &QUICPacketHeaderDeleter::delete_short_header);
   }
+  return std::move(header);
 }
 
-QUICPacketHeader *
+QUICPacketHeaderUPtr
 QUICPacketHeader::build(QUICPacketType type, QUICConnectionId connection_id, QUICPacketNumber
packet_number,
                         QUICPacketNumber base_packet_number, QUICVersion version, ats_unique_buf
payload, size_t len)
 {
   QUICPacketLongHeader *long_header = quicPacketLongHeaderAllocator.alloc();
   new (long_header) QUICPacketLongHeader(type, connection_id, packet_number, base_packet_number,
version, std::move(payload), len);
-  return long_header;
+  return QUICPacketHeaderUPtr(long_header, &QUICPacketHeaderDeleter::delete_long_header);
 }
 
-QUICPacketHeader *
+QUICPacketHeaderUPtr
 QUICPacketHeader::build(QUICPacketType type, QUICKeyPhase key_phase, QUICPacketNumber packet_number,
                         QUICPacketNumber base_packet_number, ats_unique_buf payload, size_t
len)
 {
   QUICPacketShortHeader *short_header = quicPacketShortHeaderAllocator.alloc();
   new (short_header) QUICPacketShortHeader(type, key_phase, packet_number, base_packet_number,
std::move(payload), len);
-  return short_header;
+  return QUICPacketHeaderUPtr(short_header, &QUICPacketHeaderDeleter::delete_short_header);
 }
 
-QUICPacketHeader *
+QUICPacketHeaderUPtr
 QUICPacketHeader::build(QUICPacketType type, QUICKeyPhase key_phase, QUICConnectionId connection_id,
QUICPacketNumber packet_number,
                         QUICPacketNumber base_packet_number, ats_unique_buf payload, size_t
len)
 {
   QUICPacketShortHeader *short_header = quicPacketShortHeaderAllocator.alloc();
   new (short_header)
     QUICPacketShortHeader(type, key_phase, connection_id, packet_number, base_packet_number,
std::move(payload), len);
-  return short_header;
+  return QUICPacketHeaderUPtr(short_header, &QUICPacketHeaderDeleter::delete_short_header);
 }
 
-QUICPacketHeader *
+QUICPacketHeaderUPtr
 QUICPacketHeader::clone() const
 {
-  return nullptr;
+  return QUICPacketHeaderUPtr(nullptr, &QUICPacketHeaderDeleter::delete_null_header);
 }
 
 //
@@ -473,16 +475,20 @@ QUICPacketShortHeader::store(uint8_t *buf, size_t *len) const
 // QUICPacket
 //
 
-QUICPacket::QUICPacket(QUICPacketHeader *header, ats_unique_buf payload, size_t payload_len)
+QUICPacket::QUICPacket()
 {
-  this->_header       = header;
+}
+
+QUICPacket::QUICPacket(QUICPacketHeaderUPtr header, ats_unique_buf payload, size_t payload_len)
+{
+  this->_header       = std::move(header);
   this->_payload      = std::move(payload);
   this->_payload_size = payload_len;
 }
 
-QUICPacket::QUICPacket(QUICPacketHeader *header, ats_unique_buf payload, size_t payload_len,
bool retransmittable)
+QUICPacket::QUICPacket(QUICPacketHeaderUPtr header, ats_unique_buf payload, size_t payload_len,
bool retransmittable)
 {
-  this->_header             = header;
+  this->_header             = std::move(header);
   this->_payload            = std::move(payload);
   this->_payload_size       = payload_len;
   this->_is_retransmittable = retransmittable;
@@ -490,11 +496,7 @@ QUICPacket::QUICPacket(QUICPacketHeader *header, ats_unique_buf payload,
size_t
 
 QUICPacket::~QUICPacket()
 {
-  if (this->_header->has_version()) {
-    quicPacketLongHeaderAllocator.free(static_cast<QUICPacketLongHeader *>(this->_header));
-  } else {
-    quicPacketShortHeaderAllocator.free(static_cast<QUICPacketShortHeader *>(this->_header));
-  }
+  this->_header = nullptr;
 }
 
 /**
@@ -522,7 +524,7 @@ QUICPacket::packet_number() const
 const QUICPacketHeader *
 QUICPacket::header() const
 {
-  return this->_header;
+  return this->_header.get();
 }
 
 const uint8_t *
@@ -661,7 +663,7 @@ QUICPacketFactory::create(ats_unique_buf buf, size_t len, QUICPacketNumber
base_
   ats_unique_buf plain_txt = ats_unique_malloc(max_plain_txt_len);
   size_t plain_txt_len     = 0;
 
-  QUICPacketHeader *header = QUICPacketHeader::load(buf.release(), len, base_packet_number);
+  QUICPacketHeaderUPtr header = QUICPacketHeader::load(buf.release(), len, base_packet_number);
 
   if (header->has_version() && !QUICTypeUtil::is_supported_version(header->version()))
{
     // We can't decrypt packets that have unknown versions
@@ -733,7 +735,7 @@ QUICPacketFactory::create(ats_unique_buf buf, size_t len, QUICPacketNumber
base_
   QUICPacket *packet = nullptr;
   if (result == QUICPacketCreationResult::SUCCESS || result == QUICPacketCreationResult::UNSUPPORTED)
{
     packet = quicPacketAllocator.alloc();
-    new (packet) QUICPacket(header, std::move(plain_txt), plain_txt_len);
+    new (packet) QUICPacket(std::move(header), std::move(plain_txt), plain_txt_len);
   }
 
   return QUICPacketUPtr(packet, &QUICPacketDeleter::delete_packet);
@@ -752,30 +754,31 @@ QUICPacketFactory::create_version_negotiation_packet(const QUICPacket
*packet_se
     p += n;
   }
 
-  QUICPacketHeader *header =
+  QUICPacketHeaderUPtr header =
     QUICPacketHeader::build(QUICPacketType::VERSION_NEGOTIATION, packet_sent_by_client->connection_id(),
                             packet_sent_by_client->packet_number(), base_packet_number,
0x00, std::move(versions), len);
 
-  return QUICPacketFactory::_create_unprotected_packet(header);
+  return QUICPacketFactory::_create_unprotected_packet(std::move(header));
 }
 
 QUICPacketUPtr
 QUICPacketFactory::create_initial_packet(QUICConnectionId connection_id, QUICPacketNumber
base_packet_number, QUICVersion version,
                                          ats_unique_buf payload, size_t len)
 {
-  QUICPacketHeader *header = QUICPacketHeader::build(QUICPacketType::INITIAL, connection_id,
this->_packet_number_generator.next(),
-                                                     base_packet_number, version, std::move(payload),
len);
-  return this->_create_encrypted_packet(header, true);
+  QUICPacketHeaderUPtr header =
+    QUICPacketHeader::build(QUICPacketType::INITIAL, connection_id, this->_packet_number_generator.next(),
base_packet_number,
+                            version, std::move(payload), len);
+  return this->_create_encrypted_packet(std::move(header), true);
 }
 
 QUICPacketUPtr
 QUICPacketFactory::create_handshake_packet(QUICConnectionId connection_id, QUICPacketNumber
base_packet_number,
                                            ats_unique_buf payload, size_t len, bool retransmittable)
 {
-  QUICPacketHeader *header =
+  QUICPacketHeaderUPtr header =
     QUICPacketHeader::build(QUICPacketType::HANDSHAKE, connection_id, this->_packet_number_generator.next(),
base_packet_number,
                             this->_version, std::move(payload), len);
-  return this->_create_encrypted_packet(header, retransmittable);
+  return this->_create_encrypted_packet(std::move(header), retransmittable);
 }
 
 QUICPacketUPtr
@@ -783,10 +786,10 @@ QUICPacketFactory::create_server_protected_packet(QUICConnectionId connection_id
                                                   ats_unique_buf payload, size_t len, bool
retransmittable)
 {
   // TODO Key phase should be picked up from QUICHandshakeProtocol, probably
-  QUICPacketHeader *header =
+  QUICPacketHeaderUPtr header =
     QUICPacketHeader::build(QUICPacketType::PROTECTED, QUICKeyPhase::PHASE_0, connection_id,
this->_packet_number_generator.next(),
                             base_packet_number, std::move(payload), len);
-  return this->_create_encrypted_packet(header, retransmittable);
+  return this->_create_encrypted_packet(std::move(header), retransmittable);
 }
 
 QUICPacketUPtr
@@ -807,26 +810,26 @@ QUICPacketFactory::create_stateless_reset_packet(QUICConnectionId connection_id,
   memcpy(naked_payload + payload_len - 16, stateless_reset_token.buf(), 16);
 
   // KeyPhase won't be used
-  QUICPacketHeader *header = QUICPacketHeader::build(QUICPacketType::STATELESS_RESET, QUICKeyPhase::CLEARTEXT,
connection_id,
-                                                     random_packet_number, 0, std::move(payload),
payload_len);
-  return QUICPacketFactory::_create_unprotected_packet(header);
+  QUICPacketHeaderUPtr header = QUICPacketHeader::build(QUICPacketType::STATELESS_RESET,
QUICKeyPhase::CLEARTEXT, connection_id,
+                                                        random_packet_number, 0, std::move(payload),
payload_len);
+  return QUICPacketFactory::_create_unprotected_packet(std::move(header));
 }
 
 QUICPacketUPtr
-QUICPacketFactory::_create_unprotected_packet(QUICPacketHeader *header)
+QUICPacketFactory::_create_unprotected_packet(QUICPacketHeaderUPtr header)
 {
   ats_unique_buf cleartext = ats_unique_malloc(2048);
   size_t cleartext_len     = header->payload_size();
 
   memcpy(cleartext.get(), header->payload(), cleartext_len);
   QUICPacket *packet = quicPacketAllocator.alloc();
-  new (packet) QUICPacket(header, std::move(cleartext), cleartext_len, false);
+  new (packet) QUICPacket(std::move(header), std::move(cleartext), cleartext_len, false);
 
   return QUICPacketUPtr(packet, &QUICPacketDeleter::delete_packet);
 }
 
 QUICPacketUPtr
-QUICPacketFactory::_create_encrypted_packet(QUICPacketHeader *header, bool retransmittable)
+QUICPacketFactory::_create_encrypted_packet(QUICPacketHeaderUPtr header, bool retransmittable)
 {
   // TODO: use pmtu of UnixNetVConnection
   size_t max_cipher_txt_len = 2048;
@@ -837,7 +840,7 @@ QUICPacketFactory::_create_encrypted_packet(QUICPacketHeader *header,
bool retra
   if (this->_hs_protocol->encrypt(cipher_txt.get(), cipher_txt_len, max_cipher_txt_len,
header->payload(), header->payload_size(),
                                   header->packet_number(), header->buf(), header->size(),
header->key_phase())) {
     packet = quicPacketAllocator.alloc();
-    new (packet) QUICPacket(header, std::move(cipher_txt), cipher_txt_len, retransmittable);
+    new (packet) QUICPacket(std::move(header), std::move(cipher_txt), cipher_txt_len, retransmittable);
   }
 
   return QUICPacketUPtr(packet, &QUICPacketDeleter::delete_packet);
diff --git a/iocore/net/quic/QUICPacket.h b/iocore/net/quic/QUICPacket.h
index 2e52c6e..6c367c4 100644
--- a/iocore/net/quic/QUICPacket.h
+++ b/iocore/net/quic/QUICPacket.h
@@ -37,6 +37,18 @@
 #define QUIC_FIELD_OFFSET_PACKET_NUMBER 4
 #define QUIC_FIELD_OFFSET_PAYLOAD 5
 
+class QUICPacketHeader;
+class QUICPacket;
+class QUICPacketLongHeader;
+class QUICPacketShortHeader;
+
+extern ClassAllocator<QUICPacket> quicPacketAllocator;
+extern ClassAllocator<QUICPacketLongHeader> quicPacketLongHeaderAllocator;
+extern ClassAllocator<QUICPacketShortHeader> quicPacketShortHeaderAllocator;
+
+using QUICPacketHeaderDeleterFunc = void (*)(QUICPacketHeader *p);
+using QUICPacketHeaderUPtr        = std::unique_ptr<QUICPacketHeader, QUICPacketHeaderDeleterFunc>;
+
 class QUICPacketHeader
 {
 public:
@@ -79,7 +91,7 @@ public:
    */
   virtual void store(uint8_t *buf, size_t *len) const = 0;
 
-  QUICPacketHeader *clone() const;
+  QUICPacketHeaderUPtr clone() const;
 
   virtual bool has_key_phase() const     = 0;
   virtual bool has_connection_id() const = 0;
@@ -92,32 +104,32 @@ public:
    *
    * This creates either a QUICPacketShortHeader or a QUICPacketLongHeader.
    */
-  static QUICPacketHeader *load(const uint8_t *buf, size_t len, QUICPacketNumber base);
+  static QUICPacketHeaderUPtr load(const uint8_t *buf, size_t len, QUICPacketNumber base);
 
   /*
    * Build a QUICPacketHeader
    *
    * This creates a QUICPacketLongHeader.
    */
-  static QUICPacketHeader *build(QUICPacketType type, QUICConnectionId connection_id, QUICPacketNumber
packet_number,
-                                 QUICPacketNumber base_packet_number, QUICVersion version,
ats_unique_buf payload, size_t len);
+  static QUICPacketHeaderUPtr build(QUICPacketType type, QUICConnectionId connection_id,
QUICPacketNumber packet_number,
+                                    QUICPacketNumber base_packet_number, QUICVersion version,
ats_unique_buf payload, size_t len);
 
   /*
    * Build a QUICPacketHeader
    *
    * This creates a QUICPacketShortHeader that contains a ConnectionID.
    */
-  static QUICPacketHeader *build(QUICPacketType type, QUICKeyPhase key_phase, QUICPacketNumber
packet_number,
-                                 QUICPacketNumber base_packet_number, ats_unique_buf payload,
size_t len);
+  static QUICPacketHeaderUPtr build(QUICPacketType type, QUICKeyPhase key_phase, QUICPacketNumber
packet_number,
+                                    QUICPacketNumber base_packet_number, ats_unique_buf payload,
size_t len);
 
   /*
    * Build a QUICPacketHeader
    *
    * This creates a QUICPacketShortHeader that doesn't contain a ConnectionID..
    */
-  static QUICPacketHeader *build(QUICPacketType type, QUICKeyPhase key_phase, QUICConnectionId
connection_id,
-                                 QUICPacketNumber packet_number, QUICPacketNumber base_packet_number,
ats_unique_buf payload,
-                                 size_t len);
+  static QUICPacketHeaderUPtr build(QUICPacketType type, QUICKeyPhase key_phase, QUICConnectionId
connection_id,
+                                    QUICPacketNumber packet_number, QUICPacketNumber base_packet_number,
ats_unique_buf payload,
+                                    size_t len);
 
 protected:
   QUICPacketHeader(){};
@@ -188,10 +200,36 @@ private:
   QUICPacketShortHeaderType _packet_number_type = QUICPacketShortHeaderType::UNINITIALIZED;
 };
 
+class QUICPacketHeaderDeleter
+{
+public:
+  static void
+  delete_null_header(QUICPacketHeader *header)
+  {
+    ink_assert(header == nullptr);
+  }
+
+  static void
+  delete_long_header(QUICPacketHeader *header)
+  {
+    QUICPacketLongHeader *long_header = dynamic_cast<QUICPacketLongHeader *>(header);
+    ink_assert(long_header != nullptr);
+    quicPacketLongHeaderAllocator.free(long_header);
+  }
+
+  static void
+  delete_short_header(QUICPacketHeader *header)
+  {
+    QUICPacketShortHeader *short_header = dynamic_cast<QUICPacketShortHeader *>(header);
+    ink_assert(short_header != nullptr);
+    quicPacketShortHeaderAllocator.free(short_header);
+  }
+};
+
 class QUICPacket
 {
 public:
-  QUICPacket(){};
+  QUICPacket();
 
   /*
    * Creates a QUICPacket with a QUICPacketHeader and a buffer that contains payload
@@ -199,7 +237,7 @@ public:
    * This will be used for receiving packets. Therefore, it is expected that payload is already
decrypted.
    * However,  QUICPacket class itself doesn't care about whether the payload is protected
(encrypted) or not.
    */
-  QUICPacket(QUICPacketHeader *header, ats_unique_buf payload, size_t payload_len);
+  QUICPacket(QUICPacketHeaderUPtr header, ats_unique_buf payload, size_t payload_len);
 
   /*
    * Creates a QUICPacket with a QUICPacketHeader, a buffer that contains payload and a flag
that indicates whether the packet is
@@ -208,7 +246,7 @@ public:
    * This will be used for sending packets. Therefore, it is expected that payload is already
encrypted.
    * However, QUICPacket class itself doesn't care about whether the payload is protected
(encrypted) or not.
    */
-  QUICPacket(QUICPacketHeader *header, ats_unique_buf payload, size_t payload_len, bool retransmittable);
+  QUICPacket(QUICPacketHeaderUPtr header, ats_unique_buf payload, size_t payload_len, bool
retransmittable);
 
   ~QUICPacket();
 
@@ -249,10 +287,10 @@ public:
   LINK(QUICPacket, link);
 
 private:
-  QUICPacketHeader *_header;
-  ats_unique_buf _payload  = ats_unique_buf(nullptr, [](void *p) { ats_free(p); });
-  size_t _payload_size     = 0;
-  bool _is_retransmittable = false;
+  QUICPacketHeaderUPtr _header = QUICPacketHeaderUPtr(nullptr, &QUICPacketHeaderDeleter::delete_null_header);
+  ats_unique_buf _payload      = ats_unique_buf(nullptr, [](void *p) { ats_free(p); });
+  size_t _payload_size         = 0;
+  bool _is_retransmittable     = false;
 };
 
 class QUICPacketNumberGenerator
@@ -269,10 +307,6 @@ private:
 using QUICPacketDeleterFunc = void (*)(QUICPacket *p);
 using QUICPacketUPtr        = std::unique_ptr<QUICPacket, QUICPacketDeleterFunc>;
 
-extern ClassAllocator<QUICPacket> quicPacketAllocator;
-extern ClassAllocator<QUICPacketLongHeader> quicPacketLongHeaderAllocator;
-extern ClassAllocator<QUICPacketShortHeader> quicPacketShortHeaderAllocator;
-
 class QUICPacketDeleter
 {
 public:
@@ -313,6 +347,6 @@ private:
   QUICHandshakeProtocol *_hs_protocol = nullptr;
   QUICPacketNumberGenerator _packet_number_generator;
 
-  static QUICPacketUPtr _create_unprotected_packet(QUICPacketHeader *header);
-  QUICPacketUPtr _create_encrypted_packet(QUICPacketHeader *header, bool retransmittable);
+  static QUICPacketUPtr _create_unprotected_packet(QUICPacketHeaderUPtr header);
+  QUICPacketUPtr _create_encrypted_packet(QUICPacketHeaderUPtr header, bool retransmittable);
 };

-- 
To stop receiving notification emails like this one, please contact
scw00@apache.org.

Mime
View raw message