fix: add logging for invalid cache directory path validation #4693

philprime · 2025-01-09T14:14:03Z

📜 Description

Adds additional validation and error logging to the SDK option cacheDirectoryPath.

💡 Motivation and Context

While testing edge cases of #4636 I noticed that paths are truncated to SentryCrashFU_MAX_PATH_LENGTH, which can cause file operations to use incomplete paths if they are longer than the given limit:

sentry-cocoa/Sources/SentryCrash/Recording/Tools/SentryCrashFileUtils.h

Line 41 in 52c33ed

#define SentryCrashFU_MAX_PATH_LENGTH 500

sentry-cocoa/Sources/SentryCrash/Recording/SentryCrashReport.c

Lines 1476 to 1484 in 52c33ed

    
           static char tempPath[SentryCrashFU_MAX_PATH_LENGTH]; 
        
           strlcpy(tempPath, path, sizeof(tempPath) - 10); 
        
           strlcpy(tempPath + strlen(tempPath) - 5, ".old", 5); 
        
           SENTRY_ASYNC_SAFE_LOG_INFO("Writing recrash report to %s", path); 
        
           if (rename(path, tempPath) < 0) { 
        
               SENTRY_ASYNC_SAFE_LOG_ERROR( 
        
                   "Could not rename %s to %s: %s", path, tempPath, strerror(errno)); 
        
           }

Currently the SDK will just silently fail and stop working. SDK users should be informed that their cache directory path is invalid, so they can proactively fix it.

This issue might not apply to many users, as they most likely will set shorter paths or use OS-specific defaults.

💚 How did you test it?

Configure Sentry in the SentrySDKWrapper.swift.configureSentryOptions using these test examples:

Valid path:

options.cacheDirectoryPath = "/tmp/sentry"

A path that is too long for the system:

options.cacheDirectoryPath = "/tmp/sentry" + (0..<10)
  .map { i in "\(i)abcdefghijklmnopqrstuvwxyz" }
    .joined(separator: "-")

A path where a path component is too long:

options.cacheDirectoryPath = "/tmp/sentry/" + Array(repeating: "A", count: 256).joined() + "/B/"

Set a breakpoint in _non_thread_safe_removeFileAtPath(NSString *path) in line 52 with the SENTRY_LOG_ERROR to view the error object or just check the log output for "File name too long"

sentry-cocoa/Sources/Sentry/SentryFileManager.m

Lines 43 to 58 in 52c33ed

    
           void 
        
           _non_thread_safe_removeFileAtPath(NSString *path) 
        
           { 
        
               NSError *error = nil; 
        
               NSFileManager *fileManager = [NSFileManager defaultManager]; 
        
               if (![fileManager removeItemAtPath:path error:&error]) { 
        
                   if (error.code == NSFileNoSuchFileError) { 
        
                       SENTRY_LOG_DEBUG(@"No file to delete at %@", path); 
        
                   } else { 
        
                       SENTRY_LOG_ERROR( 
        
                           @"Error occurred while deleting file at %@ because of %@", path, error); 
        
                   } 
        
               } else { 
        
                   SENTRY_LOG_DEBUG(@"Successfully deleted file at %@", path); 
        
               } 
        
           }

With the changes of this PR, the following statement will be in the log:

The configured cache directory path looks invalid, the SDK might not be able to write reports to disk: /tmp/sentry/AAA...AAA/B/

📝 Checklist

You have to check all boxes before merging:

I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
I updated the wizard if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

There are a couple of caveats to consider:

Decide if logging via SentryLog is sufficient, because this is a critical configuration error and SentryLog only logs errors if options.debug is true
Decide if the options validation must be at every point of usage of the options.cacheDirectoryPath or if using it once during SDK startup is enough.

github-actions · 2025-01-09T14:14:48Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

github-actions · 2025-01-09T14:15:51Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

codecov · 2025-01-09T14:27:50Z

Codecov Report

Attention: Patch coverage is 82.66667% with 39 lines in your changes missing coverage. Please review.

Project coverage is 91.140%. Comparing base (f3b8999) to head (a0ff34c).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ts/SentryTests/Helper/SentryFileManagerTests.swift	80.769%	35 Missing ⚠️
Sources/Sentry/SentryFileManager.m	90.476%	4 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #4693       +/-   ##
=============================================
+ Coverage   91.082%   91.140%   +0.058%     
=============================================
  Files          624       624               
  Lines        72205     72421      +216     
  Branches     25652     26349      +697     
=============================================
+ Hits         65766     66005      +239     
+ Misses        6345      6319       -26     
- Partials        94        97        +3

Files with missing lines	Coverage Δ
Sources/Sentry/SentryClient.m	`95.184% <100.000%> (ø)`
Sources/Sentry/SentryFileManager.m	`94.772% <90.476%> (-0.475%)`	⬇️
...ts/SentryTests/Helper/SentryFileManagerTests.swift	`96.119% <80.769%> (-3.395%)`	⬇️

... and 24 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3b8999...a0ff34c. Read the comment docs.

github-actions · 2025-01-09T14:28:44Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1230.17 ms	1250.92 ms	20.75 ms
Size	22.31 KiB	771.26 KiB	748.94 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`725565a`	1234.59 ms	1250.02 ms	15.43 ms
`9883c0f`	1207.59 ms	1223.14 ms	15.55 ms
`f25febb`	1224.69 ms	1247.38 ms	22.68 ms
`939cd63`	1222.53 ms	1250.76 ms	28.23 ms
`7bc3c0d`	1212.35 ms	1228.94 ms	16.59 ms
`2124551`	1231.78 ms	1264.94 ms	33.16 ms
`b35ccd0`	1233.92 ms	1256.69 ms	22.78 ms
`c0ea6a1`	1236.42 ms	1250.94 ms	14.52 ms
`f5c35a1`	1227.98 ms	1232.84 ms	4.86 ms
`f2daa68`	1238.40 ms	1256.43 ms	18.03 ms

App size

Revision	Plain	With Sentry	Diff
`725565a`	20.76 KiB	426.11 KiB	405.35 KiB
`9883c0f`	22.85 KiB	405.47 KiB	382.62 KiB
`f25febb`	21.58 KiB	414.92 KiB	393.34 KiB
`939cd63`	21.58 KiB	424.35 KiB	402.76 KiB
`7bc3c0d`	20.76 KiB	427.35 KiB	406.59 KiB
`2124551`	22.85 KiB	411.69 KiB	388.84 KiB
`b35ccd0`	21.58 KiB	573.13 KiB	551.55 KiB
`c0ea6a1`	21.58 KiB	671.94 KiB	650.35 KiB
`f5c35a1`	22.31 KiB	768.83 KiB	746.52 KiB
`f2daa68`	21.58 KiB	418.40 KiB	396.82 KiB

Previous results on branch: philprime/cache-path-validation

Startup times

Revision	Plain	With Sentry	Diff
`8a4cf3b`	1222.12 ms	1240.29 ms	18.16 ms
`8f364c9`	1241.76 ms	1253.16 ms	11.41 ms
`39c747b`	1239.21 ms	1260.33 ms	21.12 ms
`be3f63e`	1217.27 ms	1241.61 ms	24.35 ms
`6dafec2`	1223.16 ms	1226.84 ms	3.67 ms
`ebe82bd`	1228.55 ms	1243.33 ms	14.78 ms
`e45257a`	1204.94 ms	1231.60 ms	26.66 ms

App size

Revision	Plain	With Sentry	Diff
`8a4cf3b`	22.31 KiB	771.46 KiB	749.15 KiB
`8f364c9`	22.31 KiB	769.91 KiB	747.59 KiB
`39c747b`	22.31 KiB	769.26 KiB	746.94 KiB
`be3f63e`	22.31 KiB	769.22 KiB	746.90 KiB
`6dafec2`	22.31 KiB	771.03 KiB	748.71 KiB
`ebe82bd`	22.31 KiB	768.95 KiB	746.63 KiB
`e45257a`	22.31 KiB	771.46 KiB	749.15 KiB

Sources/Sentry/SentryOptions.m

Sources/Swift/SentryOptionsValidator.swift

philprime · 2025-01-09T15:32:51Z

Regarding why I added the validation to multiple locations in the SDK:

The main indicator was the options.cacheDirectoryPath to select the positions. I agree that it is probably not necessary to add validation to all of the positions, but we need to look at all of them.

For example the PrivateSentrySDKOnly.installationID is used from the Hybrid SDKs, but not in the SDK itself.

Originally I considered only adding it to SentrySDK.startWithOptions, but then I noticed that also dependencies such as crashReporter in the SentryDependencyInjection is accessing the static options, which (if i am not mistaken) could be accessed before startWithOptions is even triggered.

Please leave comments during review which duplicates of the implementation you consider to be obsolete.

philprime · 2025-01-13T15:24:21Z

Blocked until #4707 is merged

…validation

github-actions · 2025-01-13T15:27:38Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

github-actions · 2025-01-13T16:05:56Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

philprime · 2025-01-13T16:07:10Z

I changed the implementation as discussed to use actual file errors as the indicator, instead of custom path validation.

To test set the following options and filter the output for fatal:

options.cacheDirectoryPath = "/tmp/" + Array(repeating: "A", count: 1_024).joined()

EDIT:

Regarding adding a call-to-action (CTA) to the log, so that SDK users can see that they need to change the cacheDirectoryPath:

During SDK initialization, the SentryFileManager.createPathsWithOptions defines all the working directory paths using the cacheDirectoryPath, but does not use the option afterwards anymore.

The error logging is actually happening when creating sentryPath and envelopesPath, therefore adding a notice that this is caused by an invalid cacheDirectoryPath breaks the separation-of-concern of the properties.

Therefore I propose we do not include a CTA, and only log it as an fatal error so SDK users can see what's happening. With #4707 they will be able to see fatal errors even without options.debug = true

github-actions · 2025-01-13T16:13:13Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

…validation

github-actions · 2025-01-14T10:32:22Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

philipphofmann

LGTM, with one comment.

Sources/Sentry/SentryFileManager.m

philipphofmann · 2025-01-14T14:44:40Z

Ah and what about the edge case when we potentially can't write a crash report? Do you plan on creating a follow up PR? Please don't include it in this PR.

github-actions · 2025-01-14T15:05:16Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

Sources/Sentry/SentryFileManager.m

philprime · 2025-01-14T15:08:50Z

Ah and what about the edge case when we potentially can't write a crash report? Do you plan on creating a follow up PR? Please don't include it in this PR.

If setting up the base directory fails, the SDK itself also doesn't initialize and no crashes are captured.

Therefore this PR does not directly affect that, but it could happen that writing a crash fails due to e.g. out of space. I can create another issue for that case, as we might have to discuss how to test the behavior.

brustolin

LGTM

philipphofmann · 2025-01-15T09:12:25Z

Therefore this PR does not directly affect that, but it could happen that writing a crash fails due to e.g. out of space. I can create another issue for that case, as we might have to discuss how to test the behavior.

I'm not worried out of space, but any other issue. TLDR; the SDK should validate during start if it could write a crash report. If it can't, it should log a fatal error.

philprime · 2025-01-15T09:59:29Z

Not sure if validation during startup is actual worth it. My example with the out-of-space, is something that can occur later on during app execution, rather than at the beginning, therefore validating the write capabilities at the start will not help it.

Not sure what you want me to implement now

…validation

philipphofmann · 2025-01-16T12:35:47Z

Not sure if validation during startup is actual worth it. My example with the out-of-space, is something that can occur later on during app execution, rather than at the beginning, therefore validating the write capabilities at the start will not help it.

Not sure what you want me to implement now

Nothing, just add the tests and let's merge it. It's an super edge case we don't have to deal now with or we fix it in a follow up PR.

github-actions · 2025-01-16T12:40:51Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

github-actions · 2025-01-16T12:45:09Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

philipphofmann

LGTM, I think we should still add one more test. Thanks

Sources/Sentry/include/SentryFileManager.h

Sources/Sentry/SentryFileManager.m

github-actions · 2025-01-16T15:41:44Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

github-actions · 2025-01-16T16:12:57Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

philipphofmann

Thanks for adding the extra tests, LTRM

SentryTestUtils/SentryFileManager+Test.h

…validation

github-actions · 2025-01-17T10:10:12Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

philprime · 2025-01-17T10:28:52Z

Had to catch another error on older OS versions.
I believe the test cases are fine and this is ready to be merged.

fix: add logging for invalid cache-path-validation

52c33ed

add changelog

40333c6

philprime changed the title ~~fix: add logging for invalid cache-path-validation~~ fix: add logging for invalid cache directory path validation Jan 9, 2025

philprime marked this pull request as ready for review January 9, 2025 14:18

philprime requested review from philipphofmann, brustolin and armcknight as code owners January 9, 2025 14:18

philprime self-assigned this Jan 9, 2025

philipphofmann reviewed Jan 9, 2025

View reviewed changes

philprime marked this pull request as draft January 13, 2025 15:24

Merge remote-tracking branch 'origin/main' into philprime/cache-path-…

6f1e759

…validation

philprime added 2 commits January 13, 2025 16:31

revert changes

f8f2d1f

add error logging

b112c20

add missing availability checks

188ce44

philprime marked this pull request as ready for review January 13, 2025 16:16

Merge remote-tracking branch 'origin/main' into philprime/cache-path-…

f679404

…validation

philipphofmann approved these changes Jan 14, 2025

View reviewed changes

Sources/Sentry/SentryFileManager.m Outdated Show resolved Hide resolved

fix unwrapping of underlying error

bbaa486

philprime commented Jan 14, 2025

View reviewed changes

Sources/Sentry/SentryFileManager.m Show resolved Hide resolved

brustolin approved these changes Jan 15, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into philprime/cache-path-…

0591b65

…validation

add tests for isErrorPathTooLong

d0605ac

philipphofmann approved these changes Jan 16, 2025

View reviewed changes

Sources/Sentry/include/SentryFileManager.h Outdated Show resolved Hide resolved

Sources/Sentry/SentryFileManager.m Show resolved Hide resolved

added tests for create directory

7b3f4f7

fix testing with old swift compiler

379c9ce

philipphofmann approved these changes Jan 17, 2025

View reviewed changes

SentryTestUtils/SentryFileManager+Test.h Show resolved Hide resolved

philprime added 2 commits January 17, 2025 10:53

Merge remote-tracking branch 'origin/main' into philprime/cache-path-…

207108c

…validation

add additional error detection on older OS

a0ff34c

philprime merged commit eea25a8 into main Jan 17, 2025
69 of 70 checks passed

philprime deleted the philprime/cache-path-validation branch January 17, 2025 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add logging for invalid cache directory path validation #4693

fix: add logging for invalid cache directory path validation #4693

philprime commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025

github-actions bot commented Jan 9, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: philprime/cache-path-validation

Startup times

App size

philprime commented Jan 9, 2025

philprime commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

philprime commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 14, 2025

philipphofmann left a comment

philipphofmann commented Jan 14, 2025

github-actions bot commented Jan 14, 2025

philprime commented Jan 14, 2025

brustolin left a comment

philipphofmann commented Jan 15, 2025

philprime commented Jan 15, 2025

philipphofmann commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025

github-actions bot commented Jan 16, 2025

philipphofmann left a comment

github-actions bot commented Jan 16, 2025

github-actions bot commented Jan 16, 2025

philipphofmann left a comment

github-actions bot commented Jan 17, 2025

philprime commented Jan 17, 2025

	static char tempPath[SentryCrashFU_MAX_PATH_LENGTH];
	strlcpy(tempPath, path, sizeof(tempPath) - 10);
	strlcpy(tempPath + strlen(tempPath) - 5, ".old", 5);
	SENTRY_ASYNC_SAFE_LOG_INFO("Writing recrash report to %s", path);

	if (rename(path, tempPath) < 0) {
	SENTRY_ASYNC_SAFE_LOG_ERROR(
	"Could not rename %s to %s: %s", path, tempPath, strerror(errno));
	}

	void
	_non_thread_safe_removeFileAtPath(NSString *path)
	{
	NSError *error = nil;
	NSFileManager *fileManager = [NSFileManager defaultManager];
	if (![fileManager removeItemAtPath:path error:&error]) {
	if (error.code == NSFileNoSuchFileError) {
	SENTRY_LOG_DEBUG(@"No file to delete at %@", path);
	} else {
	SENTRY_LOG_ERROR(
	@"Error occurred while deleting file at %@ because of %@", path, error);
	}
	} else {
	SENTRY_LOG_DEBUG(@"Successfully deleted file at %@", path);
	}
	}

fix: add logging for invalid cache directory path validation #4693

fix: add logging for invalid cache directory path validation #4693

Conversation

philprime commented Jan 9, 2025 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Jan 9, 2025

🚨 Detected changes in high risk code 🚨

github-actions bot commented Jan 9, 2025

🚨 Detected changes in high risk code 🚨

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

github-actions bot commented Jan 9, 2025 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: philprime/cache-path-validation

Startup times

App size

philprime commented Jan 9, 2025

philprime commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

🚨 Detected changes in high risk code 🚨

github-actions bot commented Jan 13, 2025

🚨 Detected changes in high risk code 🚨

philprime commented Jan 13, 2025 • edited Loading

github-actions bot commented Jan 13, 2025

🚨 Detected changes in high risk code 🚨

github-actions bot commented Jan 14, 2025

🚨 Detected changes in high risk code 🚨

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann commented Jan 14, 2025

github-actions bot commented Jan 14, 2025

🚨 Detected changes in high risk code 🚨

philprime commented Jan 14, 2025

brustolin left a comment

Choose a reason for hiding this comment

philipphofmann commented Jan 15, 2025

philprime commented Jan 15, 2025

philipphofmann commented Jan 16, 2025 • edited Loading

github-actions bot commented Jan 16, 2025

🚨 Detected changes in high risk code 🚨

github-actions bot commented Jan 16, 2025

🚨 Detected changes in high risk code 🚨

philipphofmann left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 16, 2025

🚨 Detected changes in high risk code 🚨

github-actions bot commented Jan 16, 2025

🚨 Detected changes in high risk code 🚨

philipphofmann left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2025

🚨 Detected changes in high risk code 🚨

philprime commented Jan 17, 2025

philprime commented Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

philprime commented Jan 13, 2025 •

edited

Loading

philipphofmann commented Jan 16, 2025 •

edited

Loading