From 33728152ca6ec874786545d854cbce68a61f07b7 Mon Sep 17 00:00:00 2001 From: Byoungchan Lee Date: Wed, 4 Aug 2021 20:54:45 +0900 Subject: [PATCH] Fix crash of ObjC SDK sLD / sRD with incorrect SDP. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are two problems with setLocalDescription / setRemoteDescription in ObjC SDK. First, RTCSessionDescription.nativeDescription returns a raw nullableSessionDescriptionInterface pointer, where sLD/sRD are calling Clone() method unconditionally, so it might crash. Second, unnecessary sLD/sRD calls Clone() of the raw pointer and does not delete it, so this pointer will leak. To solve these problems, I changed the return type of nativeDescription to std::unique_ptr and removed the call to Clone() method. Bug: webrtc:13022, webrtc:13035 Change-Id: Icbb87dda62d3a11af47ec74621cf64b8a6c05228 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/227380 Reviewed-by: Kári Helgason Reviewed-by: Mirko Bonadei Commit-Queue: Byoungchan Lee Cr-Commit-Position: refs/heads/master@{#34647} --- .../api/peerconnection/RTCPeerConnection.mm | 4 +- .../RTCSessionDescription+Private.h | 3 +- .../peerconnection/RTCSessionDescription.mm | 8 ++-- sdk/objc/unittests/RTCPeerConnectionTest.mm | 38 +++++++++++++++++++ .../unittests/RTCSessionDescriptionTest.mm | 2 +- 5 files changed, 46 insertions(+), 9 deletions(-) diff --git a/sdk/objc/api/peerconnection/RTCPeerConnection.mm b/sdk/objc/api/peerconnection/RTCPeerConnection.mm index 05fe581d08..ee40135ff0 100644 --- a/sdk/objc/api/peerconnection/RTCPeerConnection.mm +++ b/sdk/objc/api/peerconnection/RTCPeerConnection.mm @@ -581,7 +581,7 @@ void PeerConnectionDelegateAdapter::OnRemoveTrack( RTC_DCHECK(completionHandler != nil); rtc::scoped_refptr observer( new rtc::RefCountedObject<::SetSessionDescriptionObserver>(completionHandler)); - _peerConnection->SetLocalDescription(sdp.nativeDescription->Clone(), observer); + _peerConnection->SetLocalDescription(sdp.nativeDescription, observer); } - (void)setLocalDescriptionWithCompletionHandler: @@ -597,7 +597,7 @@ void PeerConnectionDelegateAdapter::OnRemoveTrack( RTC_DCHECK(completionHandler != nil); rtc::scoped_refptr observer( new rtc::RefCountedObject<::SetSessionDescriptionObserver>(completionHandler)); - _peerConnection->SetRemoteDescription(sdp.nativeDescription->Clone(), observer); + _peerConnection->SetRemoteDescription(sdp.nativeDescription, observer); } - (BOOL)setBweMinBitrateBps:(nullable NSNumber *)minBitrateBps diff --git a/sdk/objc/api/peerconnection/RTCSessionDescription+Private.h b/sdk/objc/api/peerconnection/RTCSessionDescription+Private.h index 0f0a06a887..aa087e557f 100644 --- a/sdk/objc/api/peerconnection/RTCSessionDescription+Private.h +++ b/sdk/objc/api/peerconnection/RTCSessionDescription+Private.h @@ -22,7 +22,8 @@ NS_ASSUME_NONNULL_BEGIN * RTCSessionDescription object. This is needed to pass to the underlying C++ * APIs. */ - @property(nonatomic, readonly, nullable) webrtc::SessionDescriptionInterface *nativeDescription; + @property(nonatomic, + readonly) std::unique_ptr nativeDescription; /** * Initialize an RTCSessionDescription from a native diff --git a/sdk/objc/api/peerconnection/RTCSessionDescription.mm b/sdk/objc/api/peerconnection/RTCSessionDescription.mm index 9fd97fee23..4ff02e8411 100644 --- a/sdk/objc/api/peerconnection/RTCSessionDescription.mm +++ b/sdk/objc/api/peerconnection/RTCSessionDescription.mm @@ -46,13 +46,11 @@ #pragma mark - Private -- (webrtc::SessionDescriptionInterface *)nativeDescription { +- (std::unique_ptr)nativeDescription { webrtc::SdpParseError error; - webrtc::SessionDescriptionInterface *description = - webrtc::CreateSessionDescription([[self class] stdStringForType:_type], - _sdp.stdString, - &error); + std::unique_ptr description(webrtc::CreateSessionDescription( + [[self class] stdStringForType:_type], _sdp.stdString, &error)); if (!description) { RTCLogError(@"Failed to create session description: %s\nline: %s", diff --git a/sdk/objc/unittests/RTCPeerConnectionTest.mm b/sdk/objc/unittests/RTCPeerConnectionTest.mm index e45ca93a6c..3452b964d9 100644 --- a/sdk/objc/unittests/RTCPeerConnectionTest.mm +++ b/sdk/objc/unittests/RTCPeerConnectionTest.mm @@ -23,11 +23,13 @@ #import "api/peerconnection/RTCPeerConnection.h" #import "api/peerconnection/RTCPeerConnectionFactory+Native.h" #import "api/peerconnection/RTCPeerConnectionFactory.h" +#import "api/peerconnection/RTCSessionDescription.h" #import "helpers/NSString+StdString.h" @interface RTCPeerConnectionTest : NSObject - (void)testConfigurationGetter; - (void)testWithDependencies; +- (void)testWithInvalidSDP; @end @implementation RTCPeerConnectionTest @@ -137,6 +139,35 @@ } } +- (void)testWithInvalidSDP { + RTC_OBJC_TYPE(RTCPeerConnectionFactory) *factory = + [[RTC_OBJC_TYPE(RTCPeerConnectionFactory) alloc] init]; + + RTC_OBJC_TYPE(RTCConfiguration) *config = [[RTC_OBJC_TYPE(RTCConfiguration) alloc] init]; + RTC_OBJC_TYPE(RTCMediaConstraints) *contraints = + [[RTC_OBJC_TYPE(RTCMediaConstraints) alloc] initWithMandatoryConstraints:@{} + optionalConstraints:nil]; + RTC_OBJC_TYPE(RTCPeerConnection) *peerConnection = + [factory peerConnectionWithConfiguration:config constraints:contraints delegate:nil]; + + dispatch_semaphore_t negotiatedSem = dispatch_semaphore_create(0); + [peerConnection setRemoteDescription:[[RTC_OBJC_TYPE(RTCSessionDescription) alloc] + initWithType:RTCSdpTypeOffer + sdp:@"invalid"] + completionHandler:^(NSError *error) { + ASSERT_NE(error, nil); + if (error != nil) { + dispatch_semaphore_signal(negotiatedSem); + } + }]; + + NSTimeInterval timeout = 5; + ASSERT_EQ( + 0, + dispatch_semaphore_wait(negotiatedSem, + dispatch_time(DISPATCH_TIME_NOW, (int64_t)(timeout * NSEC_PER_SEC)))); + [peerConnection close]; +} @end TEST(RTCPeerConnectionTest, ConfigurationGetterTest) { @@ -152,3 +183,10 @@ TEST(RTCPeerConnectionTest, TestWithDependencies) { [test testWithDependencies]; } } + +TEST(RTCPeerConnectionTest, TestWithInvalidSDP) { + @autoreleasepool { + RTCPeerConnectionTest *test = [[RTCPeerConnectionTest alloc] init]; + [test testWithInvalidSDP]; + } +} diff --git a/sdk/objc/unittests/RTCSessionDescriptionTest.mm b/sdk/objc/unittests/RTCSessionDescriptionTest.mm index ee65649cbc..25d7ffe67d 100644 --- a/sdk/objc/unittests/RTCSessionDescriptionTest.mm +++ b/sdk/objc/unittests/RTCSessionDescriptionTest.mm @@ -31,7 +31,7 @@ RTC_OBJC_TYPE(RTCSessionDescription) *description = [[RTC_OBJC_TYPE(RTCSessionDescription) alloc] initWithType:RTCSdpTypeAnswer sdp:[self sdp]]; - webrtc::SessionDescriptionInterface *nativeDescription = + std::unique_ptr nativeDescription = description.nativeDescription; EXPECT_EQ(RTCSdpTypeAnswer,