Protect against dangling pointer in syncRequest

[karlseguin]

syncRequest creates a context that lives on the function's stack. This "works" because the transfer is only expected to live during the call to syncRequest (this is the entire premise of syncRequest). But if self.tick returns an error, the function returns, potentially with transfer.req.ctx still pointing to the stack value.

This change captures a tick error and, if the sync request is still in_progress, aborts the transfer. There's maybe a world where the error is recoverable and the request could continue, but that seems error prone. AND, the most likely non-transfer error that tick would return are pretty "serious", e.g. OOM or client disconnected.

[navidemad]

Note

Verified this one fairly closely since it's a UAF fix — it holds up. The guard

• abort correctly severs the link to the stack sync_ctx, including the
  nastier case where tick fails while curl is still mid-flight. One behavioral
  question and a small doc note, neither blocking.

The fix is correct — here's the path I traced

The risk is real: sync_ctx is on syncRequest's stack (HttpClient.zig:544)
and r.ctx = &sync_ctx (:548), so if tick errors and we return without
detaching, the still-live *Transfer keeps a dangling req.ctx. The fix aborts
before returning. What makes it safe in all exit shapes:

flowchart TD
    T["tick(200) errors"] --> G{"sync_ctx.completion<br/>still in_progress?"}
    G -->|"no — transfer already completed<br/>(possibly freed this tick)"| R["return err<br/>(never touches transfer)"]
    G -->|"yes — transfer still pending"| A["transfer.abort(err)"]
    A --> RF["requestFailed → error_callback(&sync_ctx)<br/>fires synchronously — stack still valid ✓"]
    A --> D{"must_defer?<br/>_performing OR (performing &amp; conn)"}
    D -->|"no"| DE["deinit() — transfer freed inline"]
    D -->|"yes — curl mid-flight"| DI["detachInPerform: noop ALL<br/>ctx callbacks, unlink owner,<br/>defer deinit to processOneMessage"]
    DE --> S["return err — no live ref to &sync_ctx ✓"]
    DI --> S
    R --> S2["return err ✓"]

Loading

Two things I wanted to confirm and did:

• The deferred path is the load-bearing one. If tick fails while
  client.performing is still set with an in-flight conn, detachOrDeinit
  (:1295) takes the detachInPerform branch instead of freeing inline — and
  that branch noops every ctx callback (:1319-1324), so when curl later
  drains through processOneMessage, nothing re-enters &sync_ctx. So returning
  the error immediately is safe even though the transfer struct outlives the
  call.
• The in_progress re-check (:558) is doing double duty. Beyond "don't
  abort a transfer that already finished", it also guards against using a freed
  transfer: a normal completion within a tick deinits the transfer, and if
  that same tick then surfaces an unrelated error, the guard keeps us from
  calling abort on a dangling pointer. Worth keeping that comment honest about
  both reasons if you ever touch it.

One behavioral question

In the completion != .in_progress + tick errored case, a sync request that
did complete (.done, body fully received) gets discarded and we return the
tick error instead of the SyncResponse. Given the commit's reasoning (tick
errors are "serious"), I assume that's intended — just flagging that a good
response can be dropped in favor of an unrelated error.

Nit

The request ownership-contract comment (callers must not pair with their own
errdefer headers.deinit()) moved onto the now-private requestT. The public
request is the one external callers read, so it might be worth leaving a
one-line pointer there too.

[navidemad]

The public request lost its ownership-contract doc when the body moved into requestT. Since request is the entry point external callers actually read, a one-line pointer keeps the no-double-free contract discoverable:

Suggested change

	pub fn request(self: *Client, req: Request, owner: ?*Owner) !void {
	_ = try self.requestT(req, owner);
	}
	// Thin wrapper over `requestT`. See `requestT` for the ownership contract:
	// the HttpClient takes ownership of `req` (notably `req.headers`), so callers
	// must NOT add their own `errdefer headers.deinit()` — that would double-free.
	pub fn request(self: *Client, req: Request, owner: ?*Owner) !void {
	_ = try self.requestT(req, owner);
	}