Bug 1358224 - pt 3 - fix leak in RTPHeaderExtension's rid char buffer. r=drno draft
authorMichael Froman <mfroman@mozilla.com>
Thu, 27 Apr 2017 12:27:02 -0500
changeset 569672 6c65085b7b4da2f2f03930986f19547dc1115a07
parent 569671 027616406f7de586bf130aa5e4d84acf9df80072
child 626278 da8f2a4042f159bb527e74c594687677f0574be0
push id56252
push userbmo:mfroman@nostrum.com
push dateThu, 27 Apr 2017 19:55:30 +0000
reviewersdrno
bugs1358224
milestone55.0a1
Bug 1358224 - pt 3 - fix leak in RTPHeaderExtension's rid char buffer. r=drno Turns out since Firefox doesn't receive simulcast streams, we never noticed this leak. Convert RTPHeaderExtension.rid from a char* to rtc::scoped_ptr<char[]> so it gets deleted properly. This also requires a new copy constructor and assignment operator. MozReview-Commit-ID: Jh4Gp4dAl9g
media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
media/webrtc/trunk/webrtc/common_types.cc
media/webrtc/trunk/webrtc/common_types.h
media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc
media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc
--- a/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
@@ -28,17 +28,17 @@ bool MediaPipelineFilter::Filter(const w
     // Some other stream; it is possible that an SSRC has moved, so make sure
     // we don't have that SSRC in our filter any more.
     remote_ssrc_set_.erase(header.ssrc);
     return false;
   }
 
   if (header.extension.hasRID &&
       remote_rid_set_.size() &&
-      remote_rid_set_.count(header.extension.rid)) {
+      remote_rid_set_.count(header.extension.rid.get())) {
     return true;
   }
 
   if (remote_ssrc_set_.count(header.ssrc)) {
     return true;
   }
 
   // Last ditch effort...
--- a/media/webrtc/trunk/webrtc/common_types.cc
+++ b/media/webrtc/trunk/webrtc/common_types.cc
@@ -28,17 +28,45 @@ RTPHeaderExtension::RTPHeaderExtension()
       hasTransportSequenceNumber(false),
       transportSequenceNumber(0),
       hasAudioLevel(false),
       voiceActivity(false),
       audioLevel(0),
       hasVideoRotation(false),
       videoRotation(0),
       hasRID(false),
-      rid(NULL) {
+      rid(nullptr) {
+}
+
+RTPHeaderExtension::RTPHeaderExtension(const RTPHeaderExtension& rhs) {
+  *this = rhs;
+}
+
+RTPHeaderExtension&
+RTPHeaderExtension::operator=(const RTPHeaderExtension& rhs) {
+  hasTransmissionTimeOffset = rhs.hasTransmissionTimeOffset;
+  transmissionTimeOffset = rhs.transmissionTimeOffset;
+  hasAbsoluteSendTime = rhs.hasAbsoluteSendTime;
+  absoluteSendTime = rhs.absoluteSendTime;
+  hasTransportSequenceNumber = rhs.hasTransportSequenceNumber;
+  transportSequenceNumber = rhs.transportSequenceNumber;
+
+  hasAudioLevel = rhs.hasAudioLevel;
+  voiceActivity = rhs.voiceActivity;
+  audioLevel = rhs.audioLevel;
+
+  hasVideoRotation = rhs.hasVideoRotation;
+  videoRotation = rhs.videoRotation;
+
+  hasRID = rhs.hasRID;
+  if (rhs.rid) {
+    rid.reset(new char[strlen(rhs.rid.get())+1]);
+    strcpy(rid.get(), rhs.rid.get());
+  }
+  return *this;
 }
 
 RTPHeader::RTPHeader()
     : markerBit(false),
       payloadType(0),
       sequenceNumber(0),
       timestamp(0),
       ssrc(0),
--- a/media/webrtc/trunk/webrtc/common_types.h
+++ b/media/webrtc/trunk/webrtc/common_types.h
@@ -12,16 +12,17 @@
 #define WEBRTC_COMMON_TYPES_H_
 
 #include <stddef.h>
 #include <string.h>
 
 #include <string>
 #include <vector>
 
+#include "webrtc/base/scoped_ptr.h"
 #include "webrtc/typedefs.h"
 
 #if defined(_MSC_VER)
 // Disable "new behavior: elements of array will be default initialized"
 // warning. Affects OverUseDetectorOptions.
 #pragma warning(disable:4351)
 #endif
 
@@ -825,16 +826,18 @@ struct PacketTime {
                        // indicating the potential error in the |timestamp|
                        // value,in case the system is busy.
                        // For example, the time of the last select() call.
                        // If unknown, this value will be set to zero.
 };
 
 struct RTPHeaderExtension {
   RTPHeaderExtension();
+  RTPHeaderExtension(const RTPHeaderExtension& rhs);
+  RTPHeaderExtension& operator=(const RTPHeaderExtension& rhs);
 
   bool hasTransmissionTimeOffset;
   int32_t transmissionTimeOffset;
   bool hasAbsoluteSendTime;
   uint32_t absoluteSendTime;
   bool hasTransportSequenceNumber;
   uint16_t transportSequenceNumber;
 
@@ -847,17 +850,17 @@ struct RTPHeaderExtension {
   // For Coordination of Video Orientation. See
   // http://www.etsi.org/deliver/etsi_ts/126100_126199/126114/12.07.00_60/
   // ts_126114v120700p.pdf
   bool hasVideoRotation;
   uint8_t videoRotation;
 
   // RID values for simulcast; see draft-roach-avtext-rid
   bool hasRID;
-  char *rid; // UTF8 string
+  rtc::scoped_ptr<char[]> rid; // UTF8 string
 };
 
 struct RTPHeader {
   RTPHeader();
 
   bool markerBit;
   uint8_t payloadType;
   uint16_t sequenceNumber;
--- a/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc
+++ b/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc
@@ -218,20 +218,20 @@ bool RtpReceiverImpl::IncomingRtpPacket(
 
   {
     CriticalSectionScoped lock(critical_section_rtp_receiver_.get());
 
     last_receive_time_ = clock_->TimeInMilliseconds();
     last_received_payload_length_ = payload_data_length;
     // RID rarely if ever changes
     if (rtp_header.extension.hasRID &&
-        (!rid_ || strcmp(rtp_header.extension.rid, rid_) != 0)) {
+        (!rid_ || strcmp(rtp_header.extension.rid.get(), rid_) != 0)) {
       delete [] rid_;
-      rid_ = new char[strlen(rtp_header.extension.rid)+1];
-      strcpy(rid_, rtp_header.extension.rid);
+      rid_ = new char[strlen(rtp_header.extension.rid.get())+1];
+      strcpy(rid_, rtp_header.extension.rid.get());
     }
     if (in_order) {
       if (last_received_timestamp_ != rtp_header.timestamp) {
         last_received_timestamp_ = rtp_header.timestamp;
         last_received_frame_time_ms_ = clock_->TimeInMilliseconds();
       }
       last_received_sequence_number_ = rtp_header.sequenceNumber;
     }
--- a/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc
+++ b/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc
@@ -439,22 +439,19 @@ void RtpHeaderParser::ParseOneByteExtens
           // - 1. E.G. a len of 0 indicates a header data length of 1
           if ( &ptr[len + 1] > ptrRTPDataExtensionEnd ) {
             LOG(LS_WARNING) << "Extension RtpStreamId data length " << (len + 1)
               << " is longer than remaining input parse buffer "
               << static_cast<size_t>(ptrRTPDataExtensionEnd - ptr);
             return;
           }
 
-          // TODO(jesup) - avoid allocating on each packet - high watermark the
-          // RID buffer?
-          char* ptrRID = new char[len + 2];
-          memcpy(ptrRID, ptr, len + 1);
-          ptrRID[len + 1] = '\0';
-          header->extension.rid = ptrRID;
+          header->extension.rid.reset(new char[len + 2]);
+          memcpy(header->extension.rid.get(), ptr, len + 1);
+          header->extension.rid.get()[len + 1] = '\0';
           header->extension.hasRID = true;
           break;
         }
         default: {
           LOG(LS_WARNING) << "Extension type not implemented: " << type;
           return;
         }
       }