tasks: fix OOM bugs across save/load/patch/database/audio/decompress/http/wifi/steam/blissbox completion paths

LibretroAdmin · LibretroAdmin · commit 5b73ff737c97 · 2026-04-24T06:47:53.000+02:00
Swept through tasks/ looking for unchecked malloc/calloc/realloc returns followed by dereferences that would NULL-segv on OOM. Fixes span 12 files and 22 individual sites; split by file: * task_save.c (5 fixes): - task_save_handler_finished: task_data calloc was unchecked before a memcpy(task_data, state, ...) that NULL-derefs on OOM. Now NULL-checked; on failure the task_set_data attachment is skipped and save_state_cb / undo_save_state_cb consume NULL via their own NULL checks (added below). - save_state_cb (consumer for task_save_handler_finished): previously assumed task_data non-NULL and called strdup(state->path) unconditionally. Now early-returns on NULL task_data. - content_load_state_cb (consumer for task_load_handler_ finished): previously dereferenced load_data->size / ->data at variable initialisation time. Now NULL-checks first, hoisting the field reads past the guard. - content_load_and_save_state_cb: previously dereferenced load_data->path / ->undo_data / ->undo_size / ->flags. Now delegates to content_load_state_cb on NULL (which handles NULL safely) and skips the save push. - task_load_handler_finished: on calloc failure the old code early-returned without freeing the on-thread 'state' struct or setting a task error - a pre-existing leak plus silent failure. Now frees state->data + state and sets a task error so the user sees the failure. * task_translation.c (1 fix): - PNG decode path: raw_image_data malloc unchecked; the indexed write in the conversion loop NULL-derefs. NULL-check + 'goto finish' (which NULL-safely frees raw_image_data_alpha and rpng). * task_patch.c (2 fixes): - apply_patch_xdelta: *targetdata malloc unchecked; xd3_decode_memory writes into it. NULL-check + set PATCH_TARGET_ALLOC_FAILED + goto cleanup_stream (matching the explicit ENOSPC handling below). - indexed patch scan: four mallocs for name_ips_indexed / name_bps_indexed / name_ups_indexed / name_xdelta_indexed were unchecked before four strlcpy calls that NULL-deref. Joint NULL-check returns true early (same observable result as no extra patches found). * task_database.c (1 fix): - database_info_list_iterate_found_match: db_crc + entry_path_str mallocs unchecked before the '[0] = \\0;' stores that NULL-deref. Joint NULL-check frees whichever succeeded and returns SCAN_VERDICT_ERROR so the scanner moves on to the next content entry cleanly. * task_audio_mixer.c (2 fixes): - task_push_audio_mixer_load: nbio->path strdup was unchecked; downstream strdup(nbio->path) in the ogg upload handler is UB on NULL (POSIX). NULL-check + goto error. - task_push_audio_mixer_load_and_play: identical twin pattern, identical fix. (Note: these are NOT the audio_mixer UAF that was separately reverted upstream as 4be8b20. Those were in audio/audio_driver.c; these are in the task enqueue path.) * task_decompress.c (3 fixes): - file_archived: callback_error malloc unchecked before strlcpy. NULL-gate the error-string population (skipping leaves task->error NULL, which task_set_error and downstream consumers handle). - file_decompressed: same twin pattern, same fix. - task_decompress_handler_finished: data calloc unchecked before data->source_file = ... write. On OOM free source_file (which would otherwise leak via the skipped task_set_data) and set a task error. * task_database_cue.c (3 fixes): - PS2 detect_ps2_game: disc_data malloc unchecked before intfstream_read, which writes via fread (no NULL guard on destination). NULL-check + return 0, matching the PS1/PSP twin patterns above/below which already had this check. - track_extract first variant: data malloc unchecked before intfstream_read. NULL-check + goto error. - track_extract second variant: twin pattern, twin fix. * task_http.c (1 fix): - task_http_transfer_handler: http_transfer_data_t malloc unchecked before data->data / ->len / ->headers / ->status writes. On OOM free the already-fetched 'tmp' buffer (data was about to take ownership) and set a task error. Moved 'bool mute;' declaration into the success branch to avoid an unused-variable warning on the failure branch. * task_wifi.c (1 fix): - task_push_wifi_connect: task->user_data malloc unchecked before memcpy. On OOM free task->title and the task itself, return false so the caller sees a clean failure. * task_content_disc.c (1 fix): - dump-cue DUMP_STATE_WRITE_CUE: cue_data calloc unchecked before filestream_read. NULL-gate the read; cue_data is actually unused after the read (only passed to free at the bottom of this block), so skipping the read has no further consequence beyond the failed read itself. * task_autodetect_blissbox.c (2 fixes): - lp_device_path LocalAlloc unchecked before strlcpy NULL-derefs. NULL-check + 'continue' to advance the inner SetupDiEnumDeviceInterfaces 'for (i = 0; ...; i++)' loop to the next device interface. - device_path malloc unchecked before the indexed copy loop NULL-derefs. NULL-check + free the LocalAlloc'd lp_device_path + continue. Both 'continue' statements target the inner for-loop's i++ advance clause, which is the intended behaviour (skip this interface, try the next). The outer 'while (!ret)' loop increments 'index' at the bottom of its body, which is unaffected by the inner continue. * task_steam.c (1 fix): - task_push_steam_core_dlc_install: both task_init and steam_core_dlc_install_state_t calloc were unchecked before the state->app_id / ->name / ->has_downloaded and task->handler / ->state / ->title / ->progress / ->callback writes that NULL-deref. Joint NULL-check frees whichever succeeded and returns; caller is menu Steam-DLC action with no task-return channel, so return-silently matches the pre-patch failure mode (task would have crashed or silently not installed) without the crash. (This is separate from the two other steam/steam.c bugs that are blocked on an external mist.h enum value.) All fixes follow the same pattern: NULL-check immediately after the alloc, clean up partial state on failure, and propagate the error via either task_set_error, a bool/enum return, or the next-task-iteration advance (continue). Free-on-NULL paths exploit free(NULL) being a no-op to minimise conditional branches. Thread-safety: these are all on-thread modifications within the task worker or task-dispatch thread context - task_queue.c serialises completion callbacks on the main thread before the task_data memory is read, so the NULL-toleration added in completion callbacks is safe against the earlier-dispatched worker writing NULL into task_data. Scope: all fixes are local to individual task handlers/callbacks; no API changes, no header changes, no cross-subsystem impact. The task_queue.c callback dispatch contract is unchanged - callbacks receive task_data which may be NULL if the handler was unable to allocate it, and callbacks are now consistently NULL-tolerant in the cases touched here. Reachability: every fix is reachable under OOM. The more likely user-facing cases are save_state_cb / content_load_state_cb / content_load_and_save_state_cb (triggered on every save-state / load-state action), and the blissbox enumeration (triggered on every USB joypad hotplug if blissbox support is built in).
diff --git a/tasks/task_audio_mixer.c b/tasks/task_audio_mixer.c
@@ -465,6 +465,12 @@ bool task_push_audio_mixer_load_and_play(
       goto error;
 
    nbio->path         = strdup(fullpath);
+   /* NULL-check strdup: downstream strdup(nbio->path) calls at
+    * lines 72 and 429 assume this is non-NULL.  strdup(NULL) is
+    * UB per POSIX; glibc crashes.  Fail the task setup so the
+    * caller can surface the error. */
+   if (!nbio->path)
+      goto error;
 
    if (!(mixer = (struct audio_mixer_handle*)calloc(1, sizeof(*mixer))))
       goto error;
@@ -591,6 +597,11 @@ bool task_push_audio_mixer_load(
       goto error;
 
    nbio->path         = strdup(fullpath);
+   /* NULL-check strdup: see the twin check in the sibling
+    * task_push_audio_mixer_load for the reasoning.  strdup(NULL)
+    * is UB and downstream code dereferences nbio->path. */
+   if (!nbio->path)
+      goto error;
 
    if (!(mixer = (struct audio_mixer_handle*)calloc(1, sizeof(*mixer))))
       goto error;
diff --git a/tasks/task_autodetect_blissbox.c b/tasks/task_autodetect_blissbox.c
@@ -299,11 +299,31 @@ static const blissbox_pad_type_t* input_autoconfigure_get_blissbox_pad_type_win3
             size_t nLength = _tcslen(pInterfaceDetailData->DevicePath) + 1;
             lp_device_path = (TCHAR*)LocalAlloc(LPTR, nLength * sizeof(TCHAR));
 
+            /* NULL-check lp_device_path: strlcpy below NULL-derefs
+             * on LocalAlloc failure.  Skip this device; the inner
+             * 'for (i = 0; SetupDiEnumDeviceInterfaces(...); i++)'
+             * loop advances to the next interface index.  The
+             * outer 'while (!ret)' loop is unaffected (it keys off
+             * SetupDiEnumDeviceInfo returns, not this iterator). */
+            if (!lp_device_path)
+               continue;
+
             strlcpy(lp_device_path,
                   pInterfaceDetailData->DevicePath, nLength);
 
             device_path    = (char*)malloc(nLength);
 
+            /* NULL-check device_path: the indexed write below
+             * NULL-derefs on OOM.  Free the LocalAlloc'd
+             * lp_device_path we just populated and skip this
+             * device. */
+            if (!device_path)
+            {
+               LocalFree(lp_device_path);
+               lp_device_path = NULL;
+               continue;
+            }
+
             for (len = 0; len < nLength; len++)
                device_path[len] = lp_device_path[len];
 
diff --git a/tasks/task_content_disc.c b/tasks/task_content_disc.c
@@ -148,7 +148,15 @@ static void task_cdrom_dump_handler(retro_task_t *task)
             int64_t cue_size     = filestream_get_size(state->file);
             char *cue_data       = (char*)calloc(1, cue_size);
 
-            filestream_read(state->file, cue_data, cue_size);
+            /* NULL-check: filestream_read below writes into
+             * cue_data via fread which NULL-derefs on the
+             * destination pointer.  If alloc fails skip the
+             * read - cue_data is actually unused after the read
+             * (only passed to free at the bottom of this block,
+             * line ~226), so the only consequence of the skip
+             * is that the read happens to not be performed. */
+            if (cue_data)
+               filestream_read(state->file, cue_data, cue_size);
 
             state->stream        = filestream_get_vfs_handle(state->file);
             state->toc           = retro_vfs_file_get_cdrom_toc();
diff --git a/tasks/task_database.c b/tasks/task_database.c
@@ -913,6 +913,20 @@ static enum scan_verdict database_info_list_iterate_found_match(
    database_info_t *db_info_entry =
       &db_state->info->list[db_state->entry_index];
 
+   /* NULL-check both mallocs: the 'db_crc[0] = ...' /
+    * 'entry_path_str[0] = ...' writes below NULL-deref on OOM.
+    * free(NULL) in the teardown at the end of the function is
+    * safe, so we can bail early; clean up whichever succeeded
+    * and return SCAN_VERDICT_ERROR so the scanner continues
+    * with the next content entry rather than silently
+    * mismatching. */
+   if (!db_crc || !entry_path_str)
+   {
+      free(db_crc);
+      free(entry_path_str);
+      return SCAN_VERDICT_ERROR;
+   }
+
    db_crc[0]                      = '\0';
    entry_path_str[0]              = '\0';
 
diff --git a/tasks/task_database_cue.c b/tasks/task_database_cue.c
@@ -292,6 +292,12 @@ int detect_ps2_game(intfstream_t *fd, char *s, size_t len,
       return 0;
 
    disc_data = (char*)malloc(DISC_DATA_SIZE_PS2);
+   /* NULL-check: intfstream_read writes into disc_data via
+    * filestream_read (which calls fread with no NULL-guard on the
+    * dest).  Matches the NULL-checked pattern in the PS1 / PSP
+    * variants above/below. */
+   if (!disc_data)
+      return 0;
 
    if (intfstream_read(fd, disc_data, DISC_DATA_SIZE_PS2) <= 0)
    {
@@ -1515,6 +1521,10 @@ bool intfstream_file_get_serial(const char *name,
          goto error;
 
       data = (uint8_t*)malloc(size);
+      /* NULL-check: intfstream_read below would NULL-deref on
+       * OOM via filestream_read's fread. */
+      if (!data)
+         goto error;
 
       if (intfstream_read(fd, data, size) != (int64_t) size)
       {
@@ -1669,6 +1679,10 @@ bool intfstream_file_get_crc_and_size(const char *name,
          goto error;
 
       data = (uint8_t*)malloc(len);
+      /* NULL-check: intfstream_read below would NULL-deref on
+       * OOM via filestream_read's fread. */
+      if (!data)
+         goto error;
 
       if (intfstream_read(fd, data, len) != (int64_t)len)
          goto error;
diff --git a/tasks/task_decompress.c b/tasks/task_decompress.c
@@ -136,16 +136,26 @@ static int file_decompressed_subdir(const char *name,
          return 1;
 
    userdata->dec->callback_error = (char*)malloc(CALLBACK_ERROR_SIZE);
-   _len  = strlcpy(userdata->dec->callback_error,
-         "Failed to deflate ",
-         CALLBACK_ERROR_SIZE);
-   _len += strlcpy(
-         userdata->dec->callback_error + _len,
-         path,
-         CALLBACK_ERROR_SIZE - _len);
-   userdata->dec->callback_error[  _len] = '.';
-   userdata->dec->callback_error[++_len] = '\n';
-   userdata->dec->callback_error[++_len] = '\0';
+   /* NULL-check: the strlcpy below NULL-derefs on OOM.  The
+    * downstream task_set_error calls (lines 246, 274, 304) accept
+    * NULL, so skipping the error-string population leaves the task
+    * with a NULL error and the user sees no 'Failed to deflate'
+    * message - which is strictly better than segfaulting.  Any
+    * subsequent 'if (dec->callback_error)' downstream is already
+    * NULL-safe. */
+   if (userdata->dec->callback_error)
+   {
+      _len  = strlcpy(userdata->dec->callback_error,
+            "Failed to deflate ",
+            CALLBACK_ERROR_SIZE);
+      _len += strlcpy(
+            userdata->dec->callback_error + _len,
+            path,
+            CALLBACK_ERROR_SIZE - _len);
+      userdata->dec->callback_error[  _len] = '.';
+      userdata->dec->callback_error[++_len] = '\n';
+      userdata->dec->callback_error[++_len] = '\0';
+   }
 
    return 0;
 }
@@ -178,13 +188,17 @@ static int file_decompressed(const char *name, const char *valid_exts,
    }
 
    dec->callback_error = (char*)malloc(CALLBACK_ERROR_SIZE);
-   _len  = strlcpy(dec->callback_error, "Failed to deflate ",
-		   CALLBACK_ERROR_SIZE);
-   _len += strlcpy(dec->callback_error + _len,
-		   path, CALLBACK_ERROR_SIZE     - _len);
-   dec->callback_error[  _len] = '.';
-   dec->callback_error[++_len] = '\n';
-   dec->callback_error[++_len] = '\0';
+   /* NULL-check: see twin in file_archived for reasoning. */
+   if (dec->callback_error)
+   {
+      _len  = strlcpy(dec->callback_error, "Failed to deflate ",
+              CALLBACK_ERROR_SIZE);
+      _len += strlcpy(dec->callback_error + _len,
+              path, CALLBACK_ERROR_SIZE     - _len);
+      dec->callback_error[  _len] = '.';
+      dec->callback_error[++_len] = '\n';
+      dec->callback_error[++_len] = '\0';
+   }
 
    return 0;
 }
@@ -206,8 +220,22 @@ static void task_decompress_handler_finished(retro_task_t *task,
       decompress_task_data_t *data =
          (decompress_task_data_t*)calloc(1, sizeof(*data));
 
-      data->source_file = dec->source_file;
-      task_set_data(task, data);
+      /* NULL-check: the ->source_file write below NULL-derefs on
+       * OOM.  task_set_data accepts NULL, but the consumer
+       * (completion callback) would then see a NULL task_data.
+       * If data alloc fails we instead set a task error and free
+       * source_file ourselves so downstream consumers see a
+       * clean error state rather than partial success. */
+      if (data)
+      {
+         data->source_file = dec->source_file;
+         task_set_data(task, data);
+      }
+      else
+      {
+         free(dec->source_file);
+         task_set_error(task, strdup("Out of memory"));
+      }
    }
 
    if (dec->subdir)
diff --git a/tasks/task_http.c b/tasks/task_http.c
@@ -183,20 +183,35 @@ static void task_http_transfer_handler(retro_task_t *task)
       }
       else
       {
-         bool mute;
          data          = (http_transfer_data_t*)malloc(sizeof(*data));
-         data->data    = tmp;
-         data->len     = _len;
-         data->headers = net_http_headers_ex(http->handle, http->headers_accept_err);
-         data->status  = net_http_status(http->handle);
-
-         task_set_data(task, data);
-
-         mute          = ((task->flags & RETRO_TASK_FLG_MUTE) > 0);
-
-         if (!mute && net_http_error(http->handle))
-            task_set_error(task, strldup("Download failed.",
-               sizeof("Download failed.")));
+         /* NULL-check: the field writes below NULL-deref on OOM.
+          * Free the already-fetched 'tmp' buffer (which data was
+          * about to take ownership of), set a task error so the
+          * caller sees a clean failure, and skip the task_set_data
+          * attachment. */
+         if (!data)
+         {
+            if (tmp)
+               free(tmp);
+            task_set_error(task, strldup("Out of memory.",
+                  sizeof("Out of memory.")));
+         }
+         else
+         {
+            bool mute;
+            data->data    = tmp;
+            data->len     = _len;
+            data->headers = net_http_headers_ex(http->handle, http->headers_accept_err);
+            data->status  = net_http_status(http->handle);
+
+            task_set_data(task, data);
+
+            mute          = ((task->flags & RETRO_TASK_FLG_MUTE) > 0);
+
+            if (!mute && net_http_error(http->handle))
+               task_set_error(task, strldup("Download failed.",
+                  sizeof("Download failed.")));
+         }
       }
       net_http_delete(http->handle);
    }
diff --git a/tasks/task_patch.c b/tasks/task_patch.c
@@ -689,6 +689,16 @@ static enum patch_error xdelta_apply_patch(
    } while (stream.avail_in);
 
    *targetdata = (uint8_t*)malloc(*targetlength);
+   /* NULL-check: xd3_decode_memory writes into *targetdata.
+    * Passing NULL is either a crash or an opaque xdelta error
+    * depending on the library version.  On OOM set
+    * PATCH_TARGET_ALLOC_FAILED to match the explicit ENOSPC
+    * error path below and skip the decode. */
+   if (!*targetdata)
+   {
+      error_patch = PATCH_TARGET_ALLOC_FAILED;
+      goto cleanup_stream;
+   }
    switch (xd3_decode_memory(
            patchdata, patchlen,
            sourcedata, sourcelength,
@@ -918,6 +928,23 @@ bool patch_content(
        * for subsequent patches starts at 1 */
       size_t patch_index     = 1;
 
+      /* NULL-check all four mallocs together: the strlcpy calls
+       * below dereference each buffer, and the loop that follows
+       * does indexed writes into all four.  On OOM free whichever
+       * succeeded (free(NULL) is a no-op) and skip the indexed-
+       * patch scan entirely - returning true matches the behaviour
+       * when no extra indexed patches are found on disk, which is
+       * the expected outcome for nearly all content. */
+      if (   !name_ips_indexed || !name_bps_indexed
+          || !name_ups_indexed || !name_xdelta_indexed)
+      {
+         free(name_ips_indexed);
+         free(name_bps_indexed);
+         free(name_ups_indexed);
+         free(name_xdelta_indexed);
+         return true;
+      }
+
       strlcpy(name_ips_indexed, name_ips, (name_ips_len + 1) * sizeof(char));
       strlcpy(name_bps_indexed, name_bps, (name_bps_len + 1) * sizeof(char));
       strlcpy(name_ups_indexed, name_ups, (name_ups_len + 1) * sizeof(char));
diff --git a/tasks/task_save.c b/tasks/task_save.c
diff --git a/tasks/task_steam.c b/tasks/task_steam.c
diff --git a/tasks/task_translation.c b/tasks/task_translation.c
diff --git a/tasks/task_wifi.c b/tasks/task_wifi.c
