BabylonJS / BabylonJS/JsRuntimeHost

UrlLib: Apple backend use-after-free when a UrlRequest is destroyed in flight

Open
#214 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
22
Forks
23
Avg merge
3d 14h
Merged PRs (30d)
6

Description

Split out from review feedback on BabylonJS/UrlLib#37 (filed here because BabylonJS/UrlLib has Issues disabled).

## Problem

On the Apple (`NSURLSession`) backend, an in-flight `UrlRequest` that is destroyed before it settles leads to a use-after-free.

`UrlRequest_Apple.mm`'s `SendAsync()` builds a completion handler that writes `m_statusCode`, `m_headers`, `m_responseString` / `m_responseBuffer` and calls `SetError(...)` — so it implicitly captures a raw `this`. Nothing keeps the `Impl` alive for the duration of the task:

- `UrlRequest::Impl` there has no destructor.
- It never reads `m_cancellationSource`.
- It never calls `[task cancel]`.

`ImplBase::~ImplBase()` calls `Abort()`, but `Abort()` only does `m_cancellationSource.cancel()`, which this backend ignores. So destroying a `UrlRequest` neither stops the resumed `NSURLSessionDataTask` nor extends the impl's lifetime — the task keeps running and its handler later writes into freed memory.

## Impact

Any caller that drops a `UrlRequest` before it completes (e.g. abandoning a request on teardown) can corrupt whatever the freed allocation is reused for. This is not theoretical: it was observed in UrlLib CI, where an abandoned request's late failure handler wrote its error into a *subsequent* test's `Impl`, making an unrelated, previously-passing test report `NSURLErrorCancelled` (-999) with a non-empty `ErrorString`/`ErrorSymbol`.

## Notes

- Pre-existing; not introduced by #37. That PR's new test was simply the first code to exercise it, and now works around it by waiting for the request to settle before leaving scope.
- Worth auditing the other backends for the same pattern.

## Possible fixes

- Have `Impl` inherit `std::enable_shared_from_this` and capture a strong reference in the completion handler, so the impl outlives the task.
- And/or make `Abort()` actually cancel the `NSURLSessionDataTask` on this backend, honoring `m_cancellationSource`.

Contributor guide

Open the contributing guide

Research direction

Start in UrlRequest_Apple.mm at Impl::SendAsync(), then trace ImplBase::~ImplBase() and Abort() to understand the existing cancellation path. Run the affected UrlLib CI coverage for abandoned in-flight requests. Done means destroying a request cannot let an Apple task access freed state, cancellation is honored, and the relevant backend behavior is covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.