fix: use-after-free ASAN warning (#3042)

Add an argument to debug image providers to specify whether crash info should be collected for the images. Some of these API are declared in public headers, so instead of changing those by adding the parameter, I added new method declarations that the originals call through to with the appropriate parameter value to maintain the previous behavior. Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io> Co-authored-by: Dhiogo Brustolin <dhiogo.brustolin@sentry.io>
getsentry · May 25, 2023 · 9dbf743 · 9dbf743
1 parent 438e21a
commit 9dbf743
Show file tree

Hide file tree

Showing 21 changed files with 193 additions and 47 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 ### Fixes
 
 - Convert one of the two remaining usages of `sprintf` to `snprintf` (#2866)
+- Fix use-after-free ASAN warning (#3042)
 - Fix memory leaks in the profiler (#3055, #3061)
 
 ## 8.7.2

diff --git a/Samples/iOS-Swift/iOS-Swift/Base.lproj/Main.storyboard b/Samples/iOS-Swift/iOS-Swift/Base.lproj/Main.storyboard
@@ -359,10 +359,10 @@
                         <autoresizingMask key="autoresizingMask" widthSizable="YES" heightSizable="YES"/>
                         <subviews>
                             <stackView opaque="NO" contentMode="scaleToFill" distribution="fillEqually" translatesAutoresizingMaskIntoConstraints="NO" id="fn9-mQ-2PY">
-                                <rect key="frame" x="25" y="235.5" width="270" height="112"/>
+                                <rect key="frame" x="25" y="232.5" width="270" height="118.5"/>
                                 <subviews>
                                     <stackView opaque="NO" contentMode="scaleToFill" axis="vertical" distribution="equalSpacing" translatesAutoresizingMaskIntoConstraints="NO" id="Q2k-6Y-RnD">
-                                        <rect key="frame" x="0.0" y="0.0" width="135" height="112"/>
+                                        <rect key="frame" x="0.0" y="0.0" width="135" height="118.5"/>
                                         <subviews>
                                             <button opaque="NO" contentMode="scaleToFill" contentHorizontalAlignment="center" contentVerticalAlignment="center" buttonType="system" lineBreakMode="middleTruncation" translatesAutoresizingMaskIntoConstraints="NO" id="NFC-V3-lgW">
                                                 <rect key="frame" x="0.0" y="0.0" width="135" height="28"/>
@@ -373,23 +373,23 @@
                                                 </connections>
                                             </button>
                                             <button opaque="NO" contentMode="scaleToFill" contentHorizontalAlignment="center" contentVerticalAlignment="center" buttonType="system" lineBreakMode="middleTruncation" translatesAutoresizingMaskIntoConstraints="NO" id="Llv-Yz-cwF">
-                                                <rect key="frame" x="0.0" y="28" width="135" height="28"/>
+                                                <rect key="frame" x="0.0" y="30" width="135" height="28"/>
                                                 <fontDescription key="fontDescription" type="system" pointSize="13"/>
                                                 <state key="normal" title="Capture NSException"/>
                                                 <connections>
                                                     <action selector="captureNSException:" destination="QmU-DD-itF" eventType="touchUpInside" id="Gdl-wk-dUU"/>
                                                 </connections>
                                             </button>
                                             <button opaque="NO" contentMode="scaleToFill" contentHorizontalAlignment="center" contentVerticalAlignment="center" buttonType="system" lineBreakMode="middleTruncation" translatesAutoresizingMaskIntoConstraints="NO" id="wJ4-gS-64G" userLabel="fatalError">
-                                                <rect key="frame" x="0.0" y="56" width="135" height="28"/>
+                                                <rect key="frame" x="0.0" y="60" width="135" height="28"/>
                                                 <fontDescription key="fontDescription" type="system" pointSize="13"/>
                                                 <state key="normal" title="Throw FatalError"/>
                                                 <connections>
                                                     <action selector="captureFatalError:" destination="QmU-DD-itF" eventType="touchUpInside" id="dM5-xq-lrm"/>
                                                 </connections>
                                             </button>
                                             <button opaque="NO" contentMode="scaleToFill" contentHorizontalAlignment="center" contentVerticalAlignment="center" buttonType="system" lineBreakMode="middleTruncation" translatesAutoresizingMaskIntoConstraints="NO" id="NYx-6R-0bb">
-                                                <rect key="frame" x="0.0" y="84" width="135" height="28"/>
+                                                <rect key="frame" x="0.0" y="90.5" width="135" height="28"/>
                                                 <fontDescription key="fontDescription" type="system" pointSize="13"/>
                                                 <state key="normal" title="OOM crash"/>
                                                 <connections>
@@ -399,7 +399,7 @@
                                         </subviews>
                                     </stackView>
                                     <stackView opaque="NO" contentMode="scaleToFill" axis="vertical" distribution="equalSpacing" translatesAutoresizingMaskIntoConstraints="NO" id="2CV-VI-MDY">
-                                        <rect key="frame" x="135" y="0.0" width="135" height="112"/>
+                                        <rect key="frame" x="135" y="0.0" width="135" height="118.5"/>
                                         <subviews>
                                             <button opaque="NO" contentMode="scaleToFill" contentHorizontalAlignment="center" contentVerticalAlignment="center" buttonType="system" lineBreakMode="middleTruncation" translatesAutoresizingMaskIntoConstraints="NO" id="Zbo-2T-1Zm">
                                                 <rect key="frame" x="0.0" y="0.0" width="135" height="28"/>
@@ -411,36 +411,56 @@
                                                 </connections>
                                             </button>
                                             <button opaque="NO" contentMode="scaleToFill" contentHorizontalAlignment="center" contentVerticalAlignment="center" buttonType="system" lineBreakMode="middleTruncation" translatesAutoresizingMaskIntoConstraints="NO" id="s6w-E3-5yE" userLabel="DiskWriteException">
-                                                <rect key="frame" x="0.0" y="42" width="135" height="28"/>
+                                                <rect key="frame" x="0.0" y="28" width="135" height="28"/>
                                                 <fontDescription key="fontDescription" type="system" pointSize="13"/>
                                                 <state key="normal" title="DiskWriteException"/>
                                                 <connections>
                                                     <action selector="diskWriteException:" destination="QmU-DD-itF" eventType="touchUpInside" id="p74-qC-kH7"/>
                                                 </connections>
                                             </button>
                                             <button opaque="NO" contentMode="scaleToFill" contentHorizontalAlignment="center" contentVerticalAlignment="center" buttonType="system" lineBreakMode="middleTruncation" translatesAutoresizingMaskIntoConstraints="NO" id="NNd-Ec-zXw">
-                                                <rect key="frame" x="0.0" y="84" width="135" height="28"/>
+                                                <rect key="frame" x="0.0" y="56" width="135" height="28"/>
                                                 <fontDescription key="fontDescription" type="system" pointSize="13"/>
                                                 <state key="normal" title="Crash the app"/>
                                                 <connections>
                                                     <action selector="crash:" destination="QmU-DD-itF" eventType="touchUpInside" id="qlZ-bL-KUT"/>
                                                 </connections>
                                             </button>
+                                            <button opaque="NO" contentMode="scaleToFill" contentHorizontalAlignment="center" contentVerticalAlignment="center" buttonType="system" lineBreakMode="middleTruncation" translatesAutoresizingMaskIntoConstraints="NO" id="wJs-Av-cg2">
+                                                <rect key="frame" x="0.0" y="84" width="135" height="34.5"/>
+                                                <state key="normal" title="Button"/>
+                                                <buttonConfiguration key="configuration" style="plain" title="Use-after-free"/>
+                                                <connections>
+                                                    <action selector="useAfterFree:" destination="QmU-DD-itF" eventType="touchUpInside" id="CBM-tY-0bu"/>
+                                                </connections>
+                                            </button>
                                         </subviews>
                                     </stackView>
                                 </subviews>
                             </stackView>
+                            <imageView clipsSubviews="YES" userInteractionEnabled="NO" contentMode="scaleAspectFit" horizontalHuggingPriority="251" verticalHuggingPriority="251" translatesAutoresizingMaskIntoConstraints="NO" id="1zm-ho-TCr">
+                                <rect key="frame" x="40" y="227.5" width="240" height="128"/>
+                                <constraints>
+                                    <constraint firstAttribute="width" constant="240" id="5Ge-8E-fcn"/>
+                                    <constraint firstAttribute="height" constant="128" id="lc6-97-p3W"/>
+                                </constraints>
+                            </imageView>
                         </subviews>
                         <viewLayoutGuide key="safeArea" id="RXi-Ac-EvL"/>
                         <color key="backgroundColor" systemColor="systemBackgroundColor"/>
                         <constraints>
                             <constraint firstItem="fn9-mQ-2PY" firstAttribute="centerX" secondItem="RXi-Ac-EvL" secondAttribute="centerX" id="6Jl-qx-lzY"/>
+                            <constraint firstItem="1zm-ho-TCr" firstAttribute="centerY" secondItem="RXi-Ac-EvL" secondAttribute="centerY" id="JyK-Aq-Hqe"/>
                             <constraint firstItem="fn9-mQ-2PY" firstAttribute="centerY" secondItem="RXi-Ac-EvL" secondAttribute="centerY" id="UYM-w4-XaB"/>
+                            <constraint firstItem="1zm-ho-TCr" firstAttribute="centerX" secondItem="RXi-Ac-EvL" secondAttribute="centerX" id="z3k-vH-nPR"/>
                         </constraints>
                     </view>
                     <tabBarItem key="tabBarItem" title="Errors" image="exclamationmark.triangle.fill" catalog="system" id="u05-ZO-4bg">
                         <imageReference key="selectedImage" image="exclamationmark.triangle.fill" catalog="system"/>
                     </tabBarItem>
+                    <connections>
+                        <outlet property="imageView" destination="1zm-ho-TCr" id="JvD-Tf-Vwc"/>
+                    </connections>
                 </viewController>
                 <placeholder placeholderIdentifier="IBFirstResponder" id="0Fg-3B-Ecy" userLabel="First Responder" customClass="UIResponder" sceneMemberID="firstResponder"/>
             </objects>

diff --git a/Samples/iOS-Swift/iOS-Swift/ErrorsViewController.swift b/Samples/iOS-Swift/iOS-Swift/ErrorsViewController.swift
@@ -4,6 +4,7 @@ import UIKit
 
 class ErrorsViewController: UIViewController {
 
+    @IBOutlet weak var imageView: UIImageView!
     private let dispatchQueue = DispatchQueue(label: "ErrorsViewController", attributes: .concurrent)
     private let diskWriteException = DiskWriteException()
 
@@ -12,6 +13,10 @@ class ErrorsViewController: UIViewController {
         SentrySDK.reportFullyDisplayed()
     }
 
+    @IBAction func useAfterFree(_ sender: UIButton) {
+        imageView.image = UIImage(named: "")
+    }
+
     @IBAction func diskWriteException(_ sender: UIButton) {
         highlightButton(sender)
         diskWriteException.continuouslyWriteToDisk()

diff --git a/Samples/iOS-Swift/iOS-SwiftUITests/LaunchUITests.swift b/Samples/iOS-Swift/iOS-SwiftUITests/LaunchUITests.swift
@@ -98,6 +98,25 @@ class LaunchUITests: XCTestCase {
         app.buttons["Extra"].tap()
         checkSlowAndFrozenFrames()
     }
+
+    /**
+     * We received a customer report that ASAN reports a use-after-free error after
+     * calling UIImage(named:) with an empty string argument. Recording another
+     * transaction leads to the ASAN error.
+     */
+    func testUseAfterFreeAfterUIImageNamedEmptyString() throws {
+        guard #available(iOS 14, *) else {
+            throw XCTSkip("Only run for iOS 14 or later")
+        }
+
+        let app = XCUIApplication()
+
+        // this primes the state required according to the customer report, by setting a UIImageView.image property to a UIImage(named: "")
+        app/*@START_MENU_TOKEN@*/.staticTexts["Use-after-free"]/*[[".buttons[\"Use-after-free\"].staticTexts[\"Use-after-free\"]",".staticTexts[\"Use-after-free\"]"],[[[-1,1],[-1,0]]],[0]]@END_MENU_TOKEN@*/.tap()
+
+        // this causes another transaction to be recorded which hits the codepath necessary for the ASAN to trip
+        app.tabBars["Tab Bar"].buttons["Extra"].tap()
+    }
 }
 
 private extension LaunchUITests {

diff --git a/Sources/Sentry/PrivateSentrySDKOnly.m b/Sources/Sentry/PrivateSentrySDKOnly.m
@@ -41,7 +41,14 @@ + (nullable SentryEnvelope *)envelopeWithData:(NSData *)data
 
 + (NSArray<SentryDebugMeta *> *)getDebugImages
 {
-    return [[SentryDependencyContainer sharedInstance].debugImageProvider getDebugImages];
+    // maintains previous behavior for the same method call by also trying to gather crash info
+    return [self getDebugImagesCrashed:YES];
+}
+
++ (NSArray<SentryDebugMeta *> *)getDebugImagesCrashed:(BOOL)isCrash
+{
+    return [[SentryDependencyContainer sharedInstance].debugImageProvider
+        getDebugImagesCrashed:isCrash];
 }
 
 + (nullable SentryAppStartMeasurement *)appStartMeasurement

diff --git a/Sources/Sentry/Public/SentryDebugImageProvider.h b/Sources/Sentry/Public/SentryDebugImageProvider.h
@@ -7,29 +7,73 @@ NS_ASSUME_NONNULL_BEGIN
 
 /**
  * Reserved for hybrid SDKs that the debug image list for symbolication.
+ * @todo This class should be renamed to @c SentryDebugImage in a future version.
  */
 @interface SentryDebugImageProvider : NSObject
 
 - (instancetype)init;
 
 /**
  * Returns a list of debug images that are being referenced in the given threads.
- * @param threads A list of @c SentryThread that may or may not contain stacktracse.
+ * @param threads A list of @c SentryThread that may or may not contain stacktraces.
+ * @warning This assumes a crash has occurred and attempts to read the crash information from each
+ * image's data segment, which may not be present or be invalid if a crash has not actually
+ * occurred. To avoid this, use the new @c -[getDebugImagesForThreads:isCrash:] instead.
+ * @deprecated Use @c -[getDebugImagesForThreads:isCrash:] instead.
+ */
+- (NSArray<SentryDebugMeta *> *)getDebugImagesForThreads:(NSArray<SentryThread *> *)threads
+    DEPRECATED_MSG_ATTRIBUTE("Use -[getDebugImagesForThreads:isCrash:] instead.");
+
+/**
+ * Returns a list of debug images that are being referenced in the given threads.
+ * @param threads A list of @c SentryThread that may or may not contain stacktraces.
+ * @param isCrash @c YES if we're collecting binary images for a crash report, @c NO if we're
+ * gathering them for other backtrace information, like a performance transaction. If this is for a
+ * crash, each image's data section crash info is also included.
+ */
+- (NSArray<SentryDebugMeta *> *)getDebugImagesForThreads:(NSArray<SentryThread *> *)threads
+                                                 isCrash:(BOOL)isCrash;
+
+/**
+ * Returns a list of debug images that are being referenced by the given frames.
+ * @param frames A list of stack frames.
+ * @warning This assumes a crash has occurred and attempts to read the crash information from each
+ * image's data segment, which may not be present or be invalid if a crash has not actually
+ * occurred. To avoid this, use the new @c -[getDebugImagesForFrames:isCrash:] instead.
+ * @deprecated Use @c -[getDebugImagesForFrames:isCrash:] instead.
  */
-- (NSArray<SentryDebugMeta *> *)getDebugImagesForThreads:(NSArray<SentryThread *> *)threads;
+- (NSArray<SentryDebugMeta *> *)getDebugImagesForFrames:(NSArray<SentryFrame *> *)frames
+    DEPRECATED_MSG_ATTRIBUTE("Use -[getDebugImagesForFrames:isCrash:] instead.");
 
 /**
  * Returns a list of debug images that are being referenced by the given frames.
  * @param frames A list of stack frames.
+ * @param isCrash @c YES if we're collecting binary images for a crash report, @c NO if we're
+ * gathering them for other backtrace information, like a performance transaction. If this is for a
+ * crash, each image's data section crash info is also included.
  */
-- (NSArray<SentryDebugMeta *> *)getDebugImagesForFrames:(NSArray<SentryFrame *> *)frames;
+- (NSArray<SentryDebugMeta *> *)getDebugImagesForFrames:(NSArray<SentryFrame *> *)frames
+                                                isCrash:(BOOL)isCrash;
 
 /**
  * Returns the current list of debug images. Be aware that the @c SentryDebugMeta is actually
  * describing a debug image.
- * @todo This class should be renamed to @c SentryDebugImage in a future version.
+ * @warning This assumes a crash has occurred and attempts to read the crash information from each
+ * image's data segment, which may not be present or be invalid if a crash has not actually
+ * occurred. To avoid this, use the new @c -[getDebugImagesCrashed:] instead.
+ * @deprecated Use @c -[getDebugImagesCrashed:] instead.
+ */
+- (NSArray<SentryDebugMeta *> *)getDebugImages DEPRECATED_MSG_ATTRIBUTE(
+    "Use -[getDebugImagesCrashed:] instead.");
+
+/**
+ * Returns the current list of debug images. Be aware that the @c SentryDebugMeta is actually
+ * describing a debug image.
+ * @param isCrash @c YES if we're collecting binary images for a crash report, @c NO if we're
+ * gathering them for other backtrace information, like a performance transaction. If this is for a
+ * crash, each image's data section crash info is also included.
  */
-- (NSArray<SentryDebugMeta *> *)getDebugImages;
+- (NSArray<SentryDebugMeta *> *)getDebugImagesCrashed:(BOOL)isCrash;
 
 @end
 

diff --git a/Sources/Sentry/SentryClient.m b/Sources/Sentry/SentryClient.m
@@ -587,7 +587,8 @@ - (SentryEvent *_Nullable)prepareEvent:(SentryEvent *)event
         BOOL debugMetaNotAttached = !(nil != event.debugMeta && event.debugMeta.count > 0);
         if (!isCrashEvent && shouldAttachStacktrace && debugMetaNotAttached
             && event.threads != nil) {
-            event.debugMeta = [self.debugImageProvider getDebugImagesForThreads:event.threads];
+            event.debugMeta = [self.debugImageProvider getDebugImagesForThreads:event.threads
+                                                                        isCrash:NO];
         }
     }
 

diff --git a/Sources/Sentry/SentryCrashDefaultBinaryImageProvider.m b/Sources/Sentry/SentryCrashDefaultBinaryImageProvider.m
@@ -10,10 +10,10 @@ - (NSInteger)getImageCount
     return sentrycrashdl_imageCount();
 }
 
-- (SentryCrashBinaryImage)getBinaryImage:(NSInteger)index
+- (SentryCrashBinaryImage)getBinaryImage:(NSInteger)index isCrash:(BOOL)isCrash
 {
     SentryCrashBinaryImage image = { 0 };
-    sentrycrashdl_getBinaryImage((int)index, &image);
+    sentrycrashdl_getBinaryImage((int)index, &image, isCrash);
     return image;
 }