refactor: Add full nullability handling to SentryFileManager (#5737)

Author: philprime

SentryTestUtils/SentryFileManager+Test.h

@@ -8,7 +8,7 @@ NSString *_Nullable sentryGetScopedCachesDirectory(NSString *cachesDirectory);
 NSString *_Nullable sentryBuildScopedCachesDirectoryPath(NSString *cachesDirectory,
     BOOL isSandboxed, NSString *_Nullable bundleIdentifier, NSString *_Nullable lastPathComponent);
 
-SENTRY_EXTERN NSURL *launchProfileConfigFileURL(void);
+SENTRY_EXTERN NSURL *_Nullable launchProfileConfigFileURL(void);
 SENTRY_EXTERN NSURL *_Nullable sentryLaunchConfigFileURL;
 
 @interface SentryFileManager ()

Sources/Sentry/Public/SentryDefines.h

@@ -203,3 +203,20 @@ typedef void (^SentryUserFeedbackConfigurationBlock)(
     SentryUserFeedbackConfiguration *_Nonnull configuration);
 
 #endif // TARGET_OS_IOS && SENTRY_HAS_UIKIT
+
+/**
+ * `SENTRY_UNWRAP_NULLABLE` is used to unwrap a nullable pointer type to a non-nullable pointer type
+ *  It should be used after the pointer has been checked for nullability.
+ *
+ *  For example:
+ *  ```objc
+ *  id _Nullable nullablePointer = ...;
+ *  if (nullablePointer != nil) {
+ *      MyClass *_Nonnull nonNullPointer = SENTRY_UNWRAP_NULLABLE(MyClass, nullablePointer);
+ *  }
+ *  ```
+ *
+ *  We use this macro instead of directly casting to be able to find all usages of this
+ *  pattern in the codebase.
+ */
+#define SENTRY_UNWRAP_NULLABLE(type, nullable_var) (type *_Nonnull)(nullable_var)

Sources/Sentry/SentryFileManager.m

@@ -74,13 +74,13 @@
 void
 _non_thread_safe_removeFileAtPath(NSString *path)
 {
-    NSError *error = nil;
+    NSError *_Nullable error = nil;
     NSFileManager *fileManager = [NSFileManager defaultManager];
     if ([fileManager removeItemAtPath:path error:&error]) {
         SENTRY_LOG_DEBUG(@"Successfully deleted file at %@", path);
     } else if (error.code == NSFileNoSuchFileError) {
         SENTRY_LOG_DEBUG(@"No file to delete at %@", path);
-    } else if (isErrorPathTooLong(error)) {
+    } else if (error != NULL && isErrorPathTooLong(SENTRY_UNWRAP_NULLABLE(NSError, error))) {
         SENTRY_LOG_FATAL(@"Failed to remove file, path is too long: %@", path);
     } else {
         SENTRY_LOG_ERROR(@"Error occurred while deleting file at %@ because of %@", path, error);
@@ -158,7 +158,22 @@ - (void)createPathsWithOptions:(SentryOptions *)options
     SENTRY_LOG_DEBUG(@"SentryFileManager.cachePath: %@", cachePath);
 
     self.basePath = [cachePath stringByAppendingPathComponent:@"io.sentry"];
-    self.sentryPath = [self.basePath stringByAppendingPathComponent:[options.parsedDsn getHash]];
+
+    NSString *_Nullable nullableDsnHash = [options.parsedDsn getHash];
+    if (nullableDsnHash == nil) {
+        SENTRY_LOG_FATAL(@"No DSN provided, using base path for envelopes: %@", self.basePath);
+    }
+    // We decided against changing the `sentryPath` and use a null fallback instead, because this
+    // has been broken for a long time and the impact of changing the base path can result in
+    // critical issues.
+    //
+    // Instead we silence the nullability warning and let `stringByAppendingPathComponent` handle
+    // the null case.
+    //
+    // Full discussion in https://github.com/getsentry/sentry-cocoa/pull/5737
+    self.sentryPath = [self.basePath
+        stringByAppendingPathComponent:SENTRY_UNWRAP_NULLABLE(NSString, nullableDsnHash)];
+
     self.currentSessionFilePath =
         [self.sentryPath stringByAppendingPathComponent:@"session.current"];
     self.crashedSessionFilePath =
@@ -233,7 +248,7 @@ - (nullable NSString *)getEnvelopesPath:(NSString *)filePath
         return nil;
     }
 
-    NSError *error = nil;
+    NSError *_Nullable error = nil;
     NSDictionary *dict = [[NSFileManager defaultManager] attributesOfItemAtPath:fullPath
                                                                           error:&error];
     if (error != nil) {
@@ -295,7 +310,7 @@ - (void)deleteOldEnvelopeItems
 - (void)deleteAllEnvelopes
 {
     [self removeFileAtPath:self.envelopesPath];
-    NSError *error;
+    NSError *_Nullable error;
     if (!createDirectoryIfNotExists(self.envelopesPath, &error)) {
         SENTRY_LOG_ERROR(@"Couldn't create envelopes path.");
     }
@@ -355,7 +370,12 @@ - (void)storeTimestampLastInForeground:(NSDate *)timestamp
     NSString *timestampString = sentry_toIso8601String(timestamp);
     SENTRY_LOG_DEBUG(@"Persisting lastInForeground: %@", timestampString);
     @synchronized(self.lastInForegroundFilePath) {
-        if (![self writeData:[timestampString dataUsingEncoding:NSUTF8StringEncoding]
+        NSData *_Nullable nullableData = [timestampString dataUsingEncoding:NSUTF8StringEncoding];
+        if (nullableData == nil) {
+            SENTRY_LOG_ERROR(@"Failed to convert lastInForeground timestamp to data.");
+            return;
+        }
+        if (![self writeData:SENTRY_UNWRAP_NULLABLE(NSData, nullableData)
                       toPath:self.lastInForegroundFilePath]) {
             SENTRY_LOG_WARN(@"Failed to store timestamp of last foreground event.");
         }
@@ -491,7 +511,7 @@ - (NSArray *)readPreviousBreadcrumbs
                 continue;
             }
 
-            NSError *error;
+            NSError *_Nullable error;
             NSDictionary *dict = [NSJSONSerialization JSONObjectWithData:data
                                                                  options:0
                                                                    error:&error];
@@ -535,7 +555,14 @@ - (void)storeTimezoneOffset:(NSInteger)offset
     NSString *timezoneOffsetString = [NSString stringWithFormat:@"%ld", (long)offset];
     SENTRY_LOG_DEBUG(@"Persisting timezone offset: %@", timezoneOffsetString);
     @synchronized(self.timezoneOffsetFilePath) {
-        if (![self writeData:[timezoneOffsetString dataUsingEncoding:NSUTF8StringEncoding]
+        NSData *_Nullable nullableData =
+            [timezoneOffsetString dataUsingEncoding:NSUTF8StringEncoding];
+        if (nullableData == nil) {
+            SENTRY_LOG_ERROR(@"Failed to convert timezone offset to data.");
+            return;
+        }
+
+        if (![self writeData:SENTRY_UNWRAP_NULLABLE(NSData, nullableData)
                       toPath:self.timezoneOffsetFilePath]) {
             SENTRY_LOG_WARN(@"Failed to store timezone offset.");
         }
@@ -613,7 +640,7 @@ - (nullable NSData *)readDataFromPath:(NSString *)path
 
 - (BOOL)writeData:(NSData *)data toPath:(NSString *)path
 {
-    NSError *error;
+    NSError *_Nullable error;
     if (!createDirectoryIfNotExists(self.sentryPath, &error)) {
         SENTRY_LOG_ERROR(@"File I/O not available at path %@: %@", path, error);
         return NO;
@@ -645,7 +672,7 @@ - (void)removeFileAtPath:(NSString *)path
         return @[];
     }
 
-    NSError *error = nil;
+    NSError *_Nullable error = nil;
     NSArray<NSString *> *storedFiles = [fileManager contentsOfDirectoryAtPath:path error:&error];
     if (error != nil) {
         SENTRY_LOG_ERROR(@"Couldn't load files in folder %@: %@", path, error);
@@ -674,13 +701,14 @@ - (BOOL)isDirectory:(NSString *)path
         // For iOS apps and macOS apps with sandboxing, this path will be scoped for the current
         // app. For macOS apps without sandboxing, this path is not scoped and will be shared
         // between all apps.
-        NSString *_Nullable cachesDirectory
+        NSString *_Nullable nullableCachesDirectory
             = NSSearchPathForDirectoriesInDomains(NSCachesDirectory, NSUserDomainMask, YES)
                   .firstObject;
-        if (cachesDirectory == nil) {
+        if (nullableCachesDirectory == nil) {
             SENTRY_LOG_WARN(@"No caches directory location reported.");
             return;
         }
+        NSString *_Nonnull cachesDirectory = (NSString *_Nonnull)nullableCachesDirectory;
 
         // We need to ensure our own scoped directory so that this path is not shared between other
         // apps on the same system.
@@ -767,7 +795,7 @@ - (BOOL)isDirectory:(NSString *)path
         return nil;
     }
 
-    return [cachesDirectory stringByAppendingPathComponent:identifier];
+    return [cachesDirectory stringByAppendingPathComponent:(NSString *_Nonnull)identifier];
 }
 
 NSString *_Nullable sentryStaticBasePath(void)
@@ -789,7 +817,12 @@ - (BOOL)isDirectory:(NSString *)path
 void
 removeSentryStaticBasePath(void)
 {
-    _non_thread_safe_removeFileAtPath(sentryStaticBasePath());
+    NSString *_Nullable basePath = sentryStaticBasePath();
+    if (basePath == nil) {
+        SENTRY_LOG_DEBUG(@"No base path available to remove.");
+        return;
+    }
+    _non_thread_safe_removeFileAtPath((NSString *_Nonnull)basePath);
 }
 #endif // defined(SENTRY_TEST) || defined(SENTRY_TEST_CI) || defined(DEBUG)
 
@@ -808,7 +841,7 @@ - (BOOL)isDirectory:(NSString *)path
             SENTRY_LOG_WARN(@"No location available to write a launch profiling config.");
             return;
         }
-        NSError *error;
+        NSError *_Nullable error;
         if (!createDirectoryIfNotExists(basePath, &error)) {
             SENTRY_LOG_ERROR(
                 @"Can't create base path to store launch profile config file: %@", error);
@@ -824,14 +857,25 @@ - (BOOL)isDirectory:(NSString *)path
 NSDictionary<NSString *, NSNumber *> *_Nullable sentry_persistedLaunchProfileConfigurationOptions(
     void)
 {
-    NSURL *url = launchProfileConfigFileURL();
-    if (![[NSFileManager defaultManager] fileExistsAtPath:url.path]) {
+    NSURL *_Nullable url = launchProfileConfigFileURL();
+    if (url == nil) {
+        SENTRY_LOG_ERROR(@"Failed to construct the URL to retrieve launch profile configs.")
+        return nil;
+    }
+    NSString *_Nullable nullablePath = url.path;
+    if (nullablePath == nil) {
+        SENTRY_LOG_ERROR(@"Failed to construct the path to retrieve launch profile configs.")
+        return nil;
+    }
+    if (![[NSFileManager defaultManager]
+            fileExistsAtPath:SENTRY_UNWRAP_NULLABLE(NSString, nullablePath)]) {
         return nil;
     }
 
-    NSError *error;
-    NSDictionary<NSString *, NSNumber *> *config =
-        [NSDictionary<NSString *, NSNumber *> dictionaryWithContentsOfURL:url error:&error];
+    NSError *_Nullable error;
+    NSDictionary<NSString *, NSNumber *> *config = [NSDictionary<NSString *, NSNumber *>
+        dictionaryWithContentsOfURL:SENTRY_UNWRAP_NULLABLE(NSURL, url)
+                              error:&error];
 
     if (error != nil) {
         SENTRY_LOG_ERROR(
@@ -845,9 +889,14 @@ - (BOOL)isDirectory:(NSString *)path
 BOOL
 appLaunchProfileConfigFileExists(void)
 {
-    NSString *path = launchProfileConfigFileURL().path;
+    NSURL *_Nullable url = launchProfileConfigFileURL();
+    if (url == nil) {
+        SENTRY_LOG_ERROR(@"Failed to construct the URL to check for launch profile configs.")
+        return NO;
+    }
+    NSString *_Nullable path = url.path;
     if (path == nil) {
-        SENTRY_LOG_DEBUG(@"Failed to construct the path to check for launch profile configs.")
+        SENTRY_LOG_ERROR(@"Failed to construct the path to check for launch profile configs.")
         return NO;
     }
 
@@ -857,16 +906,34 @@ - (BOOL)isDirectory:(NSString *)path
 void
 writeAppLaunchProfilingConfigFile(NSMutableDictionary<NSString *, NSNumber *> *config)
 {
-    NSError *error;
-    SENTRY_LOG_DEBUG(@"Writing launch profiling config file.");
-    SENTRY_CASSERT([config writeToURL:launchProfileConfigFileURL() error:&error],
+    NSURL *_Nullable url = launchProfileConfigFileURL();
+    if (url == nil) {
+        SENTRY_LOG_ERROR(@"Failed to construct the URL to write launch profile configs.");
+        return;
+    }
+    SENTRY_LOG_DEBUG(@"Writing launch profiling config file at url %@", url);
+
+    NSError *_Nullable error;
+    SENTRY_CASSERT([config writeToURL:SENTRY_UNWRAP_NULLABLE(NSURL, url) error:&error],
         @"Failed to write launch profile config file: %@.", error);
 }
 
 void
 removeAppLaunchProfilingConfigFile(void)
 {
-    _non_thread_safe_removeFileAtPath(launchProfileConfigFileURL().path);
+    NSURL *_Nullable url = launchProfileConfigFileURL();
+    if (url == nil) {
+        SENTRY_LOG_ERROR(@"Failed to construct the URL to remove launch profile configs.");
+        return;
+    }
+    NSString *_Nullable path = url.path;
+    if (path == nil) {
+        SENTRY_LOG_ERROR(@"Failed to construct the path to remove launch profile configs.");
+        return;
+    }
+
+    SENTRY_LOG_DEBUG(@"Removing launch profiling config file at path: %@", path);
+    _non_thread_safe_removeFileAtPath(SENTRY_UNWRAP_NULLABLE(NSString, path));
 }
 #endif // SENTRY_TARGET_PROFILING_SUPPORTED
 
@@ -940,7 +1007,7 @@ - (void)moveState:(NSString *)stateFilePath toPreviousState:(NSString *)previous
     // We first need to remove the old previous state file,
     // or we can't move the current state file to it.
     [self removeFileAtPath:previousStateFilePath];
-    NSError *error = nil;
+    NSError *_Nullable error = nil;
     if (![fileManager moveItemAtPath:stateFilePath toPath:previousStateFilePath error:&error]) {
         // We don't want to log an error if the file doesn't exist.
         if (nil != error && error.code != NSFileNoSuchFileError) {

Sources/Sentry/SentryNSDataUtils.m

@@ -63,7 +63,7 @@ + (NSData *_Nullable)sentry_gzippedWithData:(NSData *)data
     if (data == nil) {
         return nil;
     }
-    NSMutableData *mutable = [NSMutableData dataWithData:data];
+    NSMutableData *mutable = [NSMutableData dataWithData:SENTRY_UNWRAP_NULLABLE(NSData, data)];
     [mutable appendBytes:"\0" length:1];
     return mutable;
 }