Merge pull request #2513 from lightpanda-io/syncRequest_uaf

Protect against dangling pointer in syncRequest

Author: karlseguin

src/browser/HttpClient.zig

@@ -441,13 +441,18 @@ pub fn _request(_: *anyopaque, transfer: *Transfer) !void {
     return transfer.client.process(transfer);
 }
 
-// Ownership contract: from the moment this function is entered, the
-// HttpClient owns `req` — specifically `req.headers` (a curl_slist).
-// On success, transfer.deinit eventually frees it. On any failure path
-// inside this function, we free it before returning the error. Callers
-// must NOT pair `request()` with their own `errdefer headers.deinit()`
-// — that's a double-free.
+// HttpClient takes ownership of req.headers; do not pair with
+// `errdefer headers.deinit()`
 pub fn request(self: *Client, req: Request, owner: ?*Owner) !void {
+    _ = try self.requestT(req, owner);
+}
+
+// Like `request`, but returns the created `*Transfer`. The caller does not own
+// the returned `*Transfer` and must thus use it with care. From the moment this
+// function is entered, the HttpClient owns `req` — specifically `req.headers`
+// On success, transfer.deinit eventually frees it. On any failure path inside
+// this function, we free it before returning the error.
+fn requestT(self: *Client, req: Request, owner: ?*Owner) !*Transfer {
     const arena = self.arena_pool.acquire(.small, "Request.arena") catch |err| {
         req.headers.deinit();
         return err;
@@ -502,6 +507,8 @@ pub fn request(self: *Client, req: Request, owner: ?*Owner) !void {
         }
         return err;
     };
+
+    return transfer;
 }
 
 const SyncContext = struct {
@@ -565,10 +572,18 @@ pub fn syncRequest(self: *Client, allocator: Allocator, req: Request) !SyncRespo
     r.done_callback = SyncContext.doneCallback;
     r.error_callback = SyncContext.errorCallback;
     r.shutdown_callback = SyncContext.shutdownCallback;
-    try self.request(r, null);
+    const transfer = try self.requestT(r, null);
 
     while (sync_ctx.completion == .in_progress) {
-        try self.tick(200, .sync_wait);
+        self.tick(200, .sync_wait) catch |err| {
+            if (sync_ctx.completion == .in_progress) {
+                // tick failed for a reason unrelated to our transfer (likely OOM or
+                // client disconnect). transfer.req.ctx points at &sync_ctx on this
+                // stack — abort to sever that reference before we return
+                transfer.abort(err);
+            }
+            return err;
+        };
     }
 
     switch (sync_ctx.completion) {