fix: Add null checks for envelope item data in session migration and serialization #6131
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
philprime
requested review from
philipphofmann,
noahsmartin and
itaybre
as code owners
🚨 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:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| SentryMigrateSessionInit.migrateSessionInit(envelope, | ||
| envelopesDirPath: "asdf", | ||
| envelopeFilePaths: []) | ||
| } |
There was a problem hiding this comment.
Bug: Test Fails Due to Incorrect Nil Handling
It looks like testWithGarbageParametersDoesNotCrash is failing prematurely. XCTUnwrap(SentrySerialization.envelope(with: Data())) causes a crash because SentrySerialization.envelope(with: Data()) returns nil for empty data. This means the test isn't actually verifying that migrateSessionInit handles a nil envelope without crashing.
❌ 9 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b0e13a7 | 1227.71 ms | 1245.88 ms | 18.16 ms |
| d06a4db | 1233.20 ms | 1243.18 ms | 9.98 ms |
| 3c86ff3 | 1227.82 ms | 1239.73 ms | 11.91 ms |
| 42cfd79 | 1222.13 ms | 1244.23 ms | 22.10 ms |
| d3b0d62 | 1234.78 ms | 1262.69 ms | 27.91 ms |
| 82f60cf | 1218.65 ms | 1238.52 ms | 19.87 ms |
| aa0b738 | 1236.78 ms | 1253.08 ms | 16.31 ms |
| ab0ba7e | 1216.08 ms | 1242.40 ms | 26.31 ms |
| 32e7197 | 1226.91 ms | 1245.48 ms | 18.56 ms |
| 67e8e3e | 1238.80 ms | 1266.02 ms | 27.22 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b0e13a7 | 23.75 KiB | 860.98 KiB | 837.23 KiB |
| d06a4db | 23.75 KiB | 913.18 KiB | 889.43 KiB |
| 3c86ff3 | 23.75 KiB | 919.69 KiB | 895.94 KiB |
| 42cfd79 | 23.75 KiB | 880.20 KiB | 856.45 KiB |
| d3b0d62 | 23.75 KiB | 913.42 KiB | 889.67 KiB |
| 82f60cf | 23.75 KiB | 913.63 KiB | 889.88 KiB |
| aa0b738 | 23.74 KiB | 872.75 KiB | 849.00 KiB |
| ab0ba7e | 23.75 KiB | 904.54 KiB | 880.79 KiB |
| 32e7197 | 23.75 KiB | 866.69 KiB | 842.94 KiB |
| 67e8e3e | 23.75 KiB | 919.88 KiB | 896.13 KiB |
| @@ -18,7 +19,13 @@ + (BOOL)migrateSessionInit:(SentryEnvelope *)envelope | |||
|
|
|||
| for (SentryEnvelopeItem *item in envelope.items) { | |||
| if ([item.header.type isEqualToString:SentryEnvelopeItemTypes.session]) { | |||
| SentrySession *session = [SentrySerialization sessionWithData:item.data]; | |||
| if (nil == item.data) { | |||
There was a problem hiding this comment.
l: I think the yoda style checks a bit confusing.
Suggested change
| if (nil == item.data) { | |
| if (item.data == nil) { |
We even have a linter for this in Swift
Line 79 in 2372f11
Comment on lines
+7
to
+15
| override func setUp() { | ||
| super.setUp() | ||
| clearTestState() | ||
| } | ||
|
|
||
| override func tearDown() { | ||
| super.tearDown() | ||
| clearTestState() | ||
| } |
There was a problem hiding this comment.
m: Do we really need to call clearTestState for all the tests? clearTestState has some side effects and it's good avoid these and extra work.
Comment on lines
+130
to
+132
| defer { | ||
| try? FileManager.default.removeItem(atPath: tempPath) | ||
| } |
There was a problem hiding this comment.
m: I would move that up a bit so it's directly below to ensure we always remove the tempPath.
let tempPath = (tempDir as NSString).appendingPathComponent(tempFile)
| let tempFile = "test_envelope.json" | ||
| let tempPath = (tempDir as NSString).appendingPathComponent(tempFile) | ||
|
|
||
| // Create a test envelope file with the same session ID but without init flag |
There was a problem hiding this comment.
l: That comment doesn't make sense. I think we can remove it.
Suggested change
| // Create a test envelope file with the same session ID but without init flag |
Comment on lines
+119
to
+128
| let testSession = SentrySession(releaseName: "1.0.0", distinctId: "test-id") | ||
| // Don't set init flag on test session | ||
| let testSessionData = try XCTUnwrap(SentrySerialization.data(with: testSession)) | ||
| let testSessionHeader = SentryEnvelopeItemHeader(type: SentryEnvelopeItemTypes.session, | ||
| length: UInt(testSessionData.count)) | ||
| let testSessionItem = SentryEnvelopeItem(header: testSessionHeader, data: testSessionData) | ||
| let testEnvelope = SentryEnvelope(header: SentryEnvelopeHeader(id: nil), singleItem: testSessionItem) | ||
| let testEnvelopeData = try XCTUnwrap(SentrySerialization.data(with: testEnvelope)) | ||
|
|
||
| try! testEnvelopeData.write(to: URL(fileURLWithPath: tempPath)) |
There was a problem hiding this comment.
l: I'm a bit confused with the naming of the variables here. What drove you to prefix this with test? It refers to the same session as above, but the name doesn't reflect that. It's quite confusing.
| } | ||
|
|
||
| @available(*, deprecated, message: "This is only marked as deprecated because enableAppLaunchProfiling is marked as deprecated. Once that is removed this can be removed.") | ||
| func testMigrateSessionInit_WithValidSessionInit_ReturnsTrue() throws { |
There was a problem hiding this comment.
m: I don't really understand why we need this test. The testMigrateSessionInit** tests in the SentryFileManager cover this use case already. What drove you to add this tests?
|
Closing in favor of #6162 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #5577
SentryMigrateSessionInit.mto check for nilitem.databefore deserialization in bothmigrateSessionInitandsetInitFlagIfContainsSameSessionId, logging a warning and skipping items with no data to prevent crashes. [1] [2]SentrySerialization.mto return nil and log an error if any envelope item has nil data during serialization, ensuring envelopes are not serialized with missing data.SentryMigrateSessionInitTests.mwith a new, comprehensive Swift test fileSentryMigrateSessionInitTests.swift, adding multiple tests to validate behavior with nil and mixed data in envelopes, and confirming correct migration logic. [1] [2]SentrySerializationTests.swiftto ensure serialization returns nil when envelope items have nil data, covering both single and mixed cases.#skip-changelog