Skip to content

Commit 19fb869

Browse files
committed
task_http: fix UAF race on t->title in task_push_http_transfer_file
task_push_http_transfer_file() was assigning t->title AFTER task_push_http_transfer_generic() had already published the task to the queue with task_queue_push(). Worker threads can pick up, execute, and finalise (free) a published task at any time -- retro_task_regular_gather() / task_queue_remove_finished() does free(task->title); free(task) once RETRO_TASK_FLG_FINISHED is set (task_queue.c:213-222). If the network request resolves to an instant failure (DNS failure, ENETUNREACH while offline, immediate cancel) or the worker simply schedules through the task quickly under load, the worker reaches free(t) before the file helper writes t->title. The post-push write is then a heap-use-after-free. Symptoms in the wild: glibc malloc_printerr / abort during download-heavy startup paths (core updater, asset/thumbnail packs), backtrace pointing into task_http_transfer_cleanup or task_queue finalise, no obvious smoking gun in the source it points at. Fix: split task_push_http_transfer_generic() into a titled variant that sets t->title before task_queue_push(). The non-titled wrapper preserves the existing behaviour for the seven other callers (POST, WebDAV verbs, content-bearing requests) which do not assign title. task_push_http_transfer_file() now builds the title string locally first and passes it through, eliminating the post-publication write. Verified with an ASan harness mirroring the wo
1 parent 644d71d commit 19fb869

1 file changed

Lines changed: 30 additions & 10 deletions

File tree

tasks/task_http.c

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -252,9 +252,10 @@ static void http_transfer_progress_cb(retro_task_t *task)
252252
#endif
253253
}
254254

255-
static void *task_push_http_transfer_generic(
255+
static void *task_push_http_transfer_generic_titled(
256256
struct http_connection_t *conn,
257257
const char *url, bool mute, bool headers_accept_err,
258+
const char *title,
258259
retro_task_callback_t cb, void *user_data)
259260
{
260261
retro_task_t *t = NULL;
@@ -321,6 +322,17 @@ static void *task_push_http_transfer_generic(
321322
else
322323
t->flags &= ~RETRO_TASK_FLG_MUTE;
323324

325+
/* Set t->title BEFORE task_queue_push(). Once the task is on the
326+
* queue it can be picked up, run, finished, and freed by a worker
327+
* thread before this function returns; any later write to t->* is
328+
* a use-after-free. This bit callers like task_push_http_transfer_file()
329+
* which previously assigned t->title after the push -- if a download
330+
* failed instantly (DNS failure, ENETUNREACH, cancelled task) the
331+
* worker could finalise and free t before t->title was written,
332+
* giving glibc a stale pointer to free or strdup later. */
333+
if (title)
334+
t->title = strdup(title);
335+
324336
task_queue_push(t);
325337

326338
return t;
@@ -334,6 +346,15 @@ static void *task_push_http_transfer_generic(
334346
return NULL;
335347
}
336348

349+
static void *task_push_http_transfer_generic(
350+
struct http_connection_t *conn,
351+
const char *url, bool mute, bool headers_accept_err,
352+
retro_task_callback_t cb, void *user_data)
353+
{
354+
return task_push_http_transfer_generic_titled(
355+
conn, url, mute, headers_accept_err, NULL, cb, user_data);
356+
}
357+
337358
void* task_push_http_transfer(const char *url, bool mute,
338359
const char *type,
339360
retro_task_callback_t cb, void *user_data)
@@ -458,17 +479,14 @@ void* task_push_http_transfer_file(const char* url, bool mute,
458479
size_t _len;
459480
const char *s = NULL;
460481
char tmp[NAME_MAX_LENGTH] = "";
461-
retro_task_t *t = NULL;
462482

463483
if (!url || !*url)
464484
return NULL;
465485

466-
if (!(t = (retro_task_t*)task_push_http_transfer_generic(
467-
/* should be using type but some callers now rely on type being ignored */
468-
net_http_connection_new(url, "GET", NULL),
469-
url, mute, false, cb, transfer_data)))
470-
return NULL;
471-
486+
/* Build the task title BEFORE pushing to the queue. See the
487+
* comment in task_push_http_transfer_generic_titled() -- writing
488+
* t->title after the push is a use-after-free if the worker
489+
* picks up and finalises the task before the write lands. */
472490
if (transfer_data)
473491
s = transfer_data->path;
474492
else
@@ -485,8 +503,10 @@ void* task_push_http_transfer_file(const char* url, bool mute,
485503

486504
strlcpy(tmp + _len, s, sizeof(tmp) - _len);
487505

488-
t->title = strdup(tmp);
489-
return t;
506+
/* should be using type but some callers now rely on type being ignored */
507+
return task_push_http_transfer_generic_titled(
508+
net_http_connection_new(url, "GET", NULL),
509+
url, mute, false, tmp, cb, transfer_data);
490510
}
491511

492512
void* task_push_http_transfer_with_user_agent(const char *url, bool mute,

0 commit comments

Comments
 (0)