Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid crashing in SentryCoreDataTracker #3152

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

bjhomer
Copy link
Contributor

@bjhomer bjhomer commented Jul 14, 2023

📜 Description

Users can pass in a nil error parameter when calling Core Data methods like save: and executeFetchRequest:. We were incorrectly handling that nil error parameter and trying to dereference it inside SENTRY_LOG_DEBUG calls. This commit fixes that by instead checking the result of the call, as recommended by the Cocoa Error Handling Guide.

💡 Motivation and Context

This fixes #3151.

💚 How did you test it?

Honestly, I didn't. But I'm making the logging logic match the same logic used in [fetchSpan finishWithStatus:]

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • 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.
  • 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

Users can pass in a `nil` error parameter when calling Core Data methods like `save:` and `executeFetchRequest:`. We were incorrectly handling that nil error parameter and trying to dereference it inside SENTRY_LOG_DEBUG calls. This commit fixes that.
@@ -67,7 +67,7 @@ - (NSArray *)managedObjectContext:(NSManagedObjectContext *)context
finishWithStatus:result == nil ? kSentrySpanStatusInternalError : kSentrySpanStatusOk];

SENTRY_LOG_DEBUG(@"SentryCoreDataTracker automatically finished span with status: %@",
error == nil ? @"ok" : @"error");
result == nil ? @"error" : @"ok");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I swapped the order of the parameters here to make the logic in the logging mirror the logic in the [span finishWithStatus:] call above.

Previously, this logging was almost always incorrect; it was just checking whether the user had passed in an error pointer. If so, it logged "error".

@@ -109,7 +109,7 @@ - (BOOL)managedObjectContext:(NSManagedObjectContext *)context
[fetchSpan finishWithStatus:result ? kSentrySpanStatusOk : kSentrySpanStatusInternalError];

SENTRY_LOG_DEBUG(@"SentryCoreDataTracker automatically finished span with status: %@",
*error == nil ? @"ok" : @"error");
result ? @"ok" : @"error");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If result is true, then the call succeeded, and we should log ok. We have no guarantee about the state of error in this case.

If result is false, then there was an error. If the user passed in an error pointer, it will now point to the error. But if the user passed in nil, then error will still be nil, and dereferencing it would cause a crash. In either case, we don't need the actual details of the error here at all; we only need to know whether an error happened, and that can be determined entirely by looking at the result.

@bjhomer
Copy link
Contributor Author

bjhomer commented Jul 14, 2023

This was partially fixed in https://github.com/getsentry/sentry-cocoa/issues/2433, but the logging still retained the old logic, so the crash remained. This PR fixes the logging logic.

@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #3152 (59161ef) into main (d760c3f) will decrease coverage by 0.008%.
The diff coverage is 100.000%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3152       +/-   ##
=============================================
- Coverage   89.133%   89.126%   -0.008%     
=============================================
  Files          502       502               
  Lines        53965     53957        -8     
  Branches     19329     19321        -8     
=============================================
- Hits         48101     48090       -11     
+ Misses        5008      4898      -110     
- Partials       856       969      +113     
Impacted Files Coverage Δ
Sources/Sentry/SentryCoreDataTracker.m 95.683% <100.000%> (ø)

... and 30 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 d760c3f...59161ef. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @bjhomer, for the fix. I will fix the Changelog and add some tests in a subsequent PR, as I want to get a hotfix out quickly.

@philipphofmann philipphofmann merged commit 4386045 into getsentry:main Jul 17, 2023
52 of 62 checks passed
philipphofmann added a commit that referenced this pull request Jul 17, 2023
Add a changelog entry and some tests for #3152.
philipphofmann added a commit that referenced this pull request Jul 18, 2023
Add a changelog entry and some tests for #3152.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SentryCoreDataTracker *still* crashes when passed a nil error parameter
2 participants