RUM-3183 fix: Fix crash on accessing `request.allHTTPHeaderFields` #1843

ncreated · 2024-05-17T09:40:31Z

What and why?

📦 🧯 Fixes #1638

As we observed in #1638, accessing request.allHTTPHeaderFields is not safe and leads to crashes with undefined root cause. To avoid this, instead we use request.value(forHTTPHeaderField:) to only read headers known by the SDK.

How?

Removed usage of .allHTTPHeaderFields by replacing it with higher-level .value(forHTTPHeaderField:) API.

I also added linter rule to avoid using allHTTPHeaderFields if possible.

Suspected problem with `.allHTTPHeaderFields` safety

I failed to reproduce the crash, although by exploring swift-corelibs-foundation code, I suspect it to relate to the Obj-c → Swift bridging, somewhere in this code. Symbols ran during the bridging correspond to some reported in #1638 stack traces. This may invalidate our earlier assumption on the problem being caused by lack of thread safety in .allHTTPHeaderFields implementation.

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference
Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

Run unit tests for Core, RUM, Trace, Logs, CR and WVT
Run unit tests for Session Replay
Run integration tests
Run smoke tests
Run tests for tools/

As observed in #1638, accessing `request.allHTTPHeaderFields` is not safe and leads to crashes with undefined root cause. To avoid this, instead we use `request.value(forHTTPHeaderField:)` to only read headers known by the SDK.

ncreated · 2024-05-17T09:46:11Z

DatadogInternal/Sources/NetworkInstrumentation/URLSession/ImmutableRequest.swift

+        var knownHTTPHeaderFields: [String: String] = [:]
+        addHeaderIfExists(request: request, field: TracingHTTPHeaders.originField, to: &knownHTTPHeaderFields)


💡 We could consider adding other known headers here, notably the ones related to distributed tracing (x-datadog-*, W3C and B3). Not doing this as these headers are not required for upstream code. The only reason for allHTTPHeaderFields existence, was accessing the x-datadog-origin header which is added here.

Alternatively, we could replace the entire knownHTTPHeaderFields prop with datadogOriginField. This way we will move even further away from the unsafe consideration of "all headers".

Open for discussion 🧠.

/concerning

This seems risky, by only looking for some headers. We will be blind where knownHTTPHeaderFields is actually used.

Can we copy all headers for instance?

Can we copy all headers for instance?

I don't see any option without using the problematic allHTTPHeaderFields API.

This seems risky, by only looking for some headers. We will be blind where knownHTTPHeaderFields is actually used.

Makes sense 👍. I changed it to the alternative approach by replacing knownHTTPHeaderFields with ddOriginHeaderValue prop.

This is those places where I prefer using assertions (debug only) but not sure how customer can selectively disable them for libraries only else it is too much of noise.

I'm okay.

datadog-datadog-prod-us1 · 2024-05-17T09:50:38Z

Datadog Report

Branch report: ncreated/RUM-3183/fix-crash-on-accessing-allHTTPHeaderFields
Commit report: fa145d1
Test service: dd-sdk-ios

✅ 0 Failed, 3222 Passed, 0 Skipped, 2m 31.53s Total Time
🔻 Test Sessions change in coverage: 9 decreased, 4 increased

🔻 Code Coverage Decreases vs Default Branch (9)

This report shows up to 5 code coverage decreases.

test DatadogCrashReportingTests tvOS 27.39% (-0.32%) - Details
test DatadogCrashReportingTests iOS 27.36% (-0.3%) - Details
test DatadogLogsTests tvOS 46.14% (-0.26%) - Details
test DatadogLogsTests iOS 46.09% (-0.26%) - Details
test DatadogWebViewTrackingTests iOS 18.31% (-0.21%) - Details

DatadogInternal/Sources/NetworkInstrumentation/URLSession/ImmutableRequest.swift

ncreated · 2024-05-20T14:04:31Z

DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionInterceptor.swift

-        guard let headers = request.allHTTPHeaderFields else {
+        guard let headers = request.allHTTPHeaderFields else { // swiftlint:disable:this unsafe_all_http_header_fields


💡 It is IMO fine to ignore this warning here as URLSessionInterceptor is only used by Alamofire extension, which should be deprecated and removed. We have no crash reports pointing this usage, hence this should be safe.

Alternatively, we could use value(forHTTPHeaderField:) to read (one by one) all headers required by DD, B3 and W3C header readers in following lines.

~~I prefer the alternative mentioned, it's clearer 👍 thanks for the change :)~~
sorry, wrong line..

Yes, this is fine 👍

maxep

🤞

maxep · 2024-05-21T07:48:22Z

DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionInterceptor.swift

-        guard let headers = request.allHTTPHeaderFields else {
+        guard let headers = request.allHTTPHeaderFields else { // swiftlint:disable:this unsafe_all_http_header_fields


~~I prefer the alternative mentioned, it's clearer 👍 thanks for the change :)~~
sorry, wrong line..

Yes, this is fine 👍

maxep · 2024-05-21T07:48:45Z

tools/lint/sources.swiftlint.yml

+    excluded_match_kinds: 
+      - comment
+      - doccomment
+    message: "`URLRequest.allHTTPHeaderFields` is considered unsafe. Prefer using `URLRequest.value(forHTTPHeaderField:)`. See full context in https://github.com/DataDog/dd-sdk-ios/pull/1843"


maxep · 2024-05-21T07:54:44Z

DatadogInternal/Sources/NetworkInstrumentation/URLSession/ImmutableRequest.swift

+/// Introduced in response to concerns raised in https://github.com/DataDog/dd-sdk-ios/issues/1638
+/// it makes a copy of request attributes, safeguarding against potential thread safety issues arising from concurrent
+/// mutations (see more context in https://github.com/DataDog/dd-sdk-ios/pull/1767 ).
+public struct ImmutableRequest {


/suggestion I guess this could be merged with URLSessionTaskInterception, or an inner type.

In this PR we just move this type to separate file (it was added earlier). While merging types is simple in code, updating tests is harder as it adds more permutations to cover. For the sake of isolated testing, let's keep it this way.

…allHTTPHeaderFields

ncreated self-assigned this May 17, 2024

ncreated force-pushed the ncreated/RUM-3183/fix-crash-on-accessing-allHTTPHeaderFields branch from cdaf482 to 1c252a0 Compare May 17, 2024 09:42

ncreated commented May 17, 2024

View reviewed changes

ncreated marked this pull request as ready for review May 17, 2024 09:46

ncreated requested review from a team as code owners May 17, 2024 09:46

ganeshnj reviewed May 17, 2024

View reviewed changes

DatadogInternal/Sources/NetworkInstrumentation/URLSession/ImmutableRequest.swift Outdated Show resolved Hide resolved

RUM-3183 CR feedback - be explicit in reading request headers

9cb2383

ncreated commented May 20, 2024

View reviewed changes

ncreated requested a review from ganeshnj May 20, 2024 14:05

maxep previously approved these changes May 21, 2024

View reviewed changes

ganeshnj previously approved these changes May 22, 2024

View reviewed changes

Merge branch 'develop' into ncreated/RUM-3183/fix-crash-on-accessing-…

8fe0422

…allHTTPHeaderFields

ncreated dismissed stale reviews from ganeshnj and maxep via 8fe0422 May 22, 2024 09:17

Merge branch 'develop' into ncreated/RUM-3183/fix-crash-on-accessing-…

fa145d1

…allHTTPHeaderFields

ncreated requested review from ganeshnj and maxep May 22, 2024 13:05

ganeshnj approved these changes May 22, 2024

View reviewed changes

maxep approved these changes May 22, 2024

View reviewed changes

ncreated merged commit 40ce508 into develop May 22, 2024
8 checks passed

ncreated deleted the ncreated/RUM-3183/fix-crash-on-accessing-allHTTPHeaderFields branch May 22, 2024 13:27

This was referenced May 29, 2024

Dogfood recent changes #1865

Merged

Release 2.12.0 #1871

Merged

ncreated mentioned this pull request Jun 26, 2024

Crash accessing URLSessionTask.currentRequest and URLRequest.allHTTPHeaderFields kean/Pulse#268

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-3183 fix: Fix crash on accessing `request.allHTTPHeaderFields` #1843

RUM-3183 fix: Fix crash on accessing `request.allHTTPHeaderFields` #1843

ncreated commented May 17, 2024 •

edited

Loading

ncreated May 17, 2024

ganeshnj May 17, 2024

ncreated May 20, 2024

ganeshnj May 22, 2024

datadog-datadog-prod-us1 bot commented May 17, 2024 •

edited

Loading

ncreated May 20, 2024

maxep May 21, 2024 •

edited

Loading

maxep left a comment

maxep May 21, 2024 •

edited

Loading

maxep May 21, 2024

maxep May 21, 2024

ncreated May 21, 2024

		var knownHTTPHeaderFields: [String: String] = [:]
		addHeaderIfExists(request: request, field: TracingHTTPHeaders.originField, to: &knownHTTPHeaderFields)

		guard let headers = request.allHTTPHeaderFields else {
		guard let headers = request.allHTTPHeaderFields else { // swiftlint:disable:this unsafe_all_http_header_fields

RUM-3183 fix: Fix crash on accessing request.allHTTPHeaderFields #1843

RUM-3183 fix: Fix crash on accessing request.allHTTPHeaderFields #1843

Conversation

ncreated commented May 17, 2024 • edited Loading

What and why?

How?

Suspected problem with .allHTTPHeaderFields safety

Review checklist

Custom CI job configuration (optional)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented May 17, 2024 • edited Loading

Datadog Report

🔻 Code Coverage Decreases vs Default Branch (9)

Choose a reason for hiding this comment

maxep May 21, 2024 • edited Loading

Choose a reason for hiding this comment

maxep left a comment

Choose a reason for hiding this comment

maxep May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RUM-3183 fix: Fix crash on accessing `request.allHTTPHeaderFields` #1843

RUM-3183 fix: Fix crash on accessing `request.allHTTPHeaderFields` #1843

ncreated commented May 17, 2024 •

edited

Loading

Suspected problem with `.allHTTPHeaderFields` safety

datadog-datadog-prod-us1 bot commented May 17, 2024 •

edited

Loading

maxep May 21, 2024 •

edited

Loading

maxep May 21, 2024 •

edited

Loading