net_http: own the dedup key, add explicit init/deinit, stream core downloads to disk

Three loose ends from the HTTP task audit.

1. The duplicate-download guard silently stopped guarding.

Both backends stored the request URL in
char connection_url[NAME_MAX_LENGTH] via strlcpy, but
task_http_finder() compares that stored copy against the raw candidate
URL.  Truncation on the stored side alone means that once a URL
reaches NAME_MAX_LENGTH -- 256, or 128 on the small-path platforms --
the two can never compare equal, so the check whose comment reads
"Concurrent download of the same file is not allowed" returned false
for every candidate.

The failure is one-directional and therefore silent: truncation cannot
conflate two distinct URLs, since the candidate is never truncated to
match, so nothing broke loudly and downloads just stopped being
deduplicated.  Playlist thumbnail URLs exceed the buffer routinely,
concatenating a long CDN base path with a percent-encoded game title.
The cost is not just bandwidth: both tasks reach their completion
callback, and cb_http_task_download_pl_thumbnail() ends in
filestream_write_file() on an identical path.  Two writers, one file.

connection_url is now a heap-owned copy of the full URL in both
backends.  NULL means "no key" and matches nothing, so an allocation
failure admits a redundant download rather than dropping one -- the
safe direction, since a dropped push strands the caller's completion
callback forever.  samples/tasks/http/http_url_dedup_test.c, which
landed red documenting this, now passes at both NAME_MAX_LENGTH
values; it keeps the old fixed-buffer shape as a second oracle so a
revert is caught.

Two small ones alongside it, both in task_http.c:
cb_http_conn_default() returned -1 on network_init() failure without
setting http->error, and the caller discards the return value -- so
the task advanced with a NULL handle, net_http_update(NULL, ...)
returned true immediately, and it finished with neither data nor an
error, which callers read as a clean success with task_data == NULL.
And HTTP_STATUS_TRANSFER_PARSE / _PARSE_FREE were never set by
anything, so the enumerators and the case handling one of them were
dead.

2. net_http_init() / net_http_deinit().

The locks guarding the DNS cache and connection pool were created
lazily on first use:

    if (!dns_cache_lock) dns_cache_lock = slock_new();

which is an unsynchronised first-use initialisation of the very lock
meant to serialise that cache.  Two threads arriving together each
create one, one store wins, and the loser goes on locking an object
nobody else holds.  It does not reproduce once the locks exist, which
is why a TSan run over concurrent transfers came back clean -- the
window is only ever the first two requests of the process.

There was also no teardown at all: pooled sockets and their SSL
contexts, cached addrinfo, the strdup'd domains and both mutexes lived
until the process exited.  LeakSan could not see any of it, since the
lists hang off file-scope globals and so count as still reachable.

net_http_init() is called from retroarch.c immediately before
task_queue_init(), which is what spawns the task thread;
net_http_deinit() after task_queue_deinit(), so no transfer can still
hold a pooled connection.  deinit detaches both lists under their lock
and drains them with it released -- net_http_resolve() takes the same
lock at the end of its run, so joining a resolver thread while holding
it would deadlock.

3. Core downloads stream to disk.

cb_http_task_core_updater_download() buffered the whole core in RAM
and wrote it in one go, which is what task_push_http_download_file()
was added to avoid.  Everything that gated the write --
path_mkdir() and the "decompression already in progress" check -- now
runs before the transfer starts rather than after it, which is the
reordering that made this worth doing separately.  The
already-decompressing check matters more in the new position, not
less: it now prevents overwriting a file another task is reading,
where before it merely declined to.

The callback keeps the output_dir computation for the decompress call
and gates on data->status, since the partial file is removed unless
the transfer finished cleanly with a 2xx.

Sample suite extended to 92 checks, including a teardown case that
deinits, re-inits and transfers again -- calling deinit is what makes
any real leak in the cache and pool visible to LeakSan.  Clean under
ASan, LSan, UBSan and TSan.  net_http.c, task_http.c and
task_core_updater.c compile clean; task_http_emscripten.c verified
with emcc 6.0.5 at -Wall; retroarch.c hunks are the two
net_http_init/deinit calls and one include.

libretro-common/include/net/net_http.h

@@ -60,6 +60,24 @@ void net_http_connection_set_headers(struct http_connection_t *conn, const char
  *
  * Runs on whichever thread drives net_http_update().
  **/
+/**
+ * net_http_init:
+ *
+ * Creates the locks guarding the process-global DNS cache and
+ * connection pool.  Call once at startup, before any thread can reach
+ * net_http_update(); they were previously created lazily on first
+ * use, which raced.  Idempotent, but not safe to call concurrently.
+ **/
+void net_http_init(void);
+
+/**
+ * net_http_deinit:
+ *
+ * Closes pooled connections and frees the DNS cache and the locks.
+ * Call at shutdown, after the last transfer has finished.
+ **/
+void net_http_deinit(void);
+
 typedef bool (*net_http_sink_t)(void *userdata, const void *data, size_t len);
 
 /**

libretro-common/net/net_http.c

@@ -1922,6 +1922,103 @@ static bool net_http_redirect(struct http_t *state, const char *location)
    return false;
 }
 
+/**
+ * net_http_init:
+ *
+ * Creates the locks guarding the process-global DNS cache and
+ * connection pool.  Must be called once, before any thread can reach
+ * net_http_update().
+ *
+ * These were created lazily on first use inside net_http_new_socket():
+ *
+ *     if (!dns_cache_lock) dns_cache_lock = slock_new();
+ *
+ * which is an unsynchronised first-use initialisation of the very
+ * lock meant to serialise that cache.  Two threads arriving together
+ * each create one, one store wins, and the loser goes on locking an
+ * object nobody else holds -- so the cache is walked and mutated with
+ * no mutual exclusion at all.  It does not reproduce once the locks
+ * exist, which is why it survived a TSan run over concurrent
+ * transfers; the window is only ever the first two requests of the
+ * process.
+ *
+ * Idempotent, so callers that cannot easily order their startup can
+ * call it more than once -- but not concurrently, which is the whole
+ * point.
+ **/
+void net_http_init(void)
+{
+#ifdef HAVE_THREADS
+   if (!dns_cache_lock)
+      dns_cache_lock = slock_new();
+   if (!conn_pool_lock)
+      conn_pool_lock = slock_new();
+#endif
+}
+
+/**
+ * net_http_deinit:
+ *
+ * Tears down the DNS cache and connection pool.  Nothing did this
+ * before: pooled sockets (and their SSL contexts), cached addrinfo,
+ * the strdup'd domains and both mutexes simply lived until the
+ * process exited.
+ *
+ * Both lists are detached under their lock and then drained with the
+ * lock released.  That ordering matters for the DNS cache:
+ * net_http_resolve() takes the same lock at the end of its run, so
+ * joining a resolver thread while holding it deadlocks.
+ **/
+void net_http_deinit(void)
+{
+   struct conn_pool_entry *conns;
+   struct dns_cache_entry *entries;
+
+   LOCK_POOL();
+   conns     = conn_pool;
+   conn_pool = NULL;
+   UNLOCK_POOL();
+
+   while (conns)
+   {
+      struct conn_pool_entry *next = conns->next;
+      net_http_conn_pool_free(conns);
+      conns = next;
+   }
+
+   LOCK_DNS_CACHE();
+   entries   = dns_cache;
+   dns_cache = NULL;
+   UNLOCK_DNS_CACHE();
+
+   while (entries)
+   {
+      struct dns_cache_entry *next = entries->next;
+#ifdef HAVE_THREADS
+      if (entries->thread)
+         sthread_join(entries->thread);
+#endif
+      if (entries->addr)
+         freeaddrinfo_retro(entries->addr);
+      free(entries->domain);
+      free(entries);
+      entries = next;
+   }
+
+#ifdef HAVE_THREADS
+   if (dns_cache_lock)
+   {
+      slock_free(dns_cache_lock);
+      dns_cache_lock = NULL;
+   }
+   if (conn_pool_lock)
+   {
+      slock_free(conn_pool_lock);
+      conn_pool_lock = NULL;
+   }
+#endif
+}
+
 /**
  * net_http_update:
  *

retroarch.c

@@ -140,6 +140,7 @@
 #ifdef HAVE_NETWORKING
 #include <net/net_compat.h>
 #include <net/net_socket.h>
+#include <net/net_http.h>
 #endif
 
 #include <audio/audio_resampler.h>
@@ -6311,6 +6312,11 @@ void main_exit(void *args)
    retroarch_ctl(RARCH_CTL_STATE_FREE,  NULL);
    global_free(p_rarch);
    task_queue_deinit();
+#ifdef HAVE_NETWORKING
+   /* After task_queue_deinit(), so no transfer can still be holding a
+    * pooled connection. */
+   net_http_deinit();
+#endif
 
    ui_companion_driver_deinit();
    retroarch_config_deinit();
@@ -8789,6 +8795,14 @@ void retroarch_init_task_queue(void)
 #endif
 
    task_queue_deinit();
+#ifdef HAVE_NETWORKING
+   /* Before task_queue_init(), which is what spawns the task thread.
+    * net_http's DNS cache and connection pool locks used to be
+    * created lazily on first use, so the first two concurrent
+    * transfers of the process could each create one and then lock
+    * different objects. */
+   net_http_init();
+#endif
    task_queue_init(threaded_enable, runloop_task_msg_queue_push);
 }
 

samples/tasks/http/http_transfer_stress_test.c

@@ -952,6 +952,55 @@ static void test_sink_write_failure(void)
    close(sp.listen_fd);
 }
 
+/* net_http_deinit() must close pooled connections and free the DNS
+ * cache and the locks.  Nothing did this before -- pooled sockets and
+ * their SSL contexts, cached addrinfo, strdup'd domains and both
+ * mutexes lived until the process exited, which LeakSan could not
+ * see because the lists hang off file-scope globals and so count as
+ * still reachable.  Calling deinit here is what makes any real leak
+ * in that state visible.
+ *
+ * Also covers re-init after deinit, since the locks are freed and
+ * nulled and a second init has to recreate them. */
+static void test_deinit_releases_globals(void)
+{
+   struct srv_spec sp;
+   pthread_t th;
+   struct xfer_result r;
+
+   printf("  deinit, then re-init and transfer again\n");
+
+   net_http_deinit();
+   net_http_init();
+
+   memset(&sp, 0, sizeof(sp));
+   sp.body  = 64 * 1024;
+   sp.frame = FRAME_LEN;
+
+   if (!srv_start(&sp, &th))
+   {
+      printf("    SKIP: server start failed\n");
+      return;
+   }
+   if (!run_transfer(sp.port, &r, 0))
+   {
+      printf("    SKIP: client setup failed\n");
+      pthread_join(th, NULL);
+      close(sp.listen_fd);
+      return;
+   }
+
+   CHECK(r.len == sp.body, "transfer after re-init got %lu of %lu bytes",
+         (unsigned long)r.len, (unsigned long)sp.body);
+
+   free(r.data);
+   pthread_join(th, NULL);
+   close(sp.listen_fd);
+
+   /* Final teardown; anything still held now is a genuine leak. */
+   net_http_deinit();
+}
+
 /* ================================================================= */
 
 int main(void)
@@ -968,6 +1017,10 @@ int main(void)
    }
 
    network_init();
+   /* Explicit init, so the DNS cache and connection pool locks exist
+    * before any thread can reach net_http_update().  They used to be
+    * created lazily on first use, unsynchronised. */
+   net_http_init();
    pattern_init(BODY_LARGE);
 
    printf("net_http transfer stress tests "
@@ -1015,6 +1068,9 @@ int main(void)
    printf("\n[concurrency]\n");
    test_concurrent();
 
+   printf("\n[teardown]\n");
+   test_deinit_releases_globals();
+
    free((void*)g_pattern);
 
    printf("\n%s (%d check%s, %d failure%s)\n",

samples/tasks/http/http_url_dedup_test.c

@@ -62,8 +62,14 @@
  * the natural trigger, since they concatenate a long CDN base path
  * with a percent-encoded game title and routinely exceed 256 bytes.
  *
+ * Fixed by making connection_url a heap-owned copy of the full URL in
+ * both backends.  This test carries both key shapes: `key_owned`
+ * mirrors what task_http.c does now and carries the real assertions,
+ * while `key_fixed` reproduces the old fixed-buffer shape so the
+ * failure mode stays documented and a revert is caught.
+ *
  * The finder predicate is static to task_http.c and not exposed in a
- * header, so this test keeps a behavioural copy as the oracle -- the
+ * header, so this test keeps behavioural copies as the oracles -- the
  * same convention http_method_match_test.c and
  * archive_name_safety_test.c use.  If task_http.c changes how the key
  * is derived, update the copy here to match.
@@ -239,14 +245,16 @@ static void test_long_url_fails_to_dedup_itself(void)
          "fixture must exceed the key buffer (len=%u, buf=%u)",
          (unsigned)strlen(url), (unsigned)NAME_MAX_LENGTH);
 
-   /* Pre-fix: the guard silently stops guarding. */
+   /* The historical failure mode, kept as the reason the production
+    * key is heap-owned: reverting connection_url to a fixed buffer
+    * reintroduces exactly this. */
    key_fixed_set(&ka, url);
-   CHECK(key_fixed_matches(&ka, same) == 1,
-         "fixed-size key fails to match its own URL -- the "
-         "\"concurrent download of the same file\" guard is disabled, "
-         "so two tasks race to filestream_write_file() the same path");
+   CHECK(key_fixed_matches(&ka, same) == 0,
+         "fixed-size key unexpectedly matched -- this oracle is meant "
+         "to reproduce the pre-fix behaviour and no longer does, so "
+         "the rest of this test is not proving what it claims");
 
-   /* Post-fix: the guard works at any length. */
+   /* Production behaviour: the guard works at any length. */
    if (key_owned_set(&kb, url))
    {
       CHECK(key_owned_matches(&kb, same) == 1,

tasks/task_core_updater.c

@@ -578,7 +578,7 @@ void cb_http_task_core_updater_download(
 
    if (!data || !transf)
       goto finish;
-   if (!data->data || !*transf->path)
+   if (!*transf->path)
       goto finish;
 
    if (!(download_handle = (core_updater_download_handle_t*)transf->user_data))
@@ -587,36 +587,25 @@ void cb_http_task_core_updater_download(
    /* Update download_handle task status */
    download_handle->http_task_complete       = true;
 
-   /* Create output directory, if required */
+   /* The body was streamed to transf->path as it arrived, so
+    * data->data is NULL by design and there is nothing to write here.
+    * task_push_http_download_file() removes the partial file unless
+    * the transfer finished cleanly, so a non-2xx means there is no
+    * file on disk to decompress. */
+   if (err && *err)
+      goto finish;
+
+   if (data->status < 200 || data->status > 299)
+   {
+      err = "Download failed.";
+      goto finish;
+   }
+
+   /* Recomputed for the decompress call below; the directory itself
+    * was created before the transfer started. */
    strlcpy(output_dir, transf->path, sizeof(output_dir));
    path_basedir_wrapper(output_dir);
 
-   if (!path_mkdir(output_dir))
-   {
-      err = msg_hash_to_str(MSG_FAILED_TO_CREATE_THE_DIRECTORY);
-      goto finish;
-   }
-
-#ifdef HAVE_COMPRESSION
-   /* If core file is an archive, make sure it is
-    * not being decompressed already (by another task) */
-   if (path_is_compressed_file(transf->path))
-   {
-      if (task_check_decompress(transf->path))
-      {
-         err = msg_hash_to_str(MSG_DECOMPRESSION_ALREADY_IN_PROGRESS);
-         goto finish;
-      }
-   }
-#endif
-
-   /* Write core file to disk */
-   if (!filestream_write_file(transf->path, data->data, data->len))
-   {
-      err = "Write failed.";
-      goto finish;
-   }
-
 #if defined(HAVE_COMPRESSION)
    /* Decompress core file, if required
     * NOTE: If core is compressed and platform
@@ -830,16 +819,65 @@ static void task_core_updater_download_handler(retro_task_t *task)
                         sizeof(file_transfer_t))))
                goto task_finished;
 
+            char output_dir[DIR_MAX_LENGTH];
+            char http_title[NAME_MAX_LENGTH];
+            size_t _tlen;
+
             strlcpy(
                   transf->path, download_handle->local_download_path,
                   sizeof(transf->path));
 
             transf->user_data = (void*)download_handle;
 
+            /* The body is streamed straight to transf->path as it
+             * arrives, so everything that used to gate the write in
+             * cb_http_task_core_updater_download() has to happen
+             * before the transfer starts rather than after it. */
+            strlcpy(output_dir, transf->path, sizeof(output_dir));
+            path_basedir_wrapper(output_dir);
+
+            if (!path_mkdir(output_dir))
+            {
+               RARCH_ERR("[Core Updater] Download of \"%s\" failed: %s.\n",
+                     transf->path,
+                     msg_hash_to_str(MSG_FAILED_TO_CREATE_THE_DIRECTORY));
+               free(transf);
+               download_handle->status = CORE_UPDATER_DOWNLOAD_ERROR;
+               break;
+            }
+
+#ifdef HAVE_COMPRESSION
+            /* If the core file is an archive, make sure it is not
+             * already being decompressed by another task -- otherwise
+             * we would now be overwriting the very file that task is
+             * reading. */
+            if (path_is_compressed_file(transf->path)
+                  && task_check_decompress(transf->path))
+            {
+               RARCH_ERR("[Core Updater] Download of \"%s\" failed: %s.\n",
+                     transf->path,
+                     msg_hash_to_str(MSG_DECOMPRESSION_ALREADY_IN_PROGRESS));
+               free(transf);
+               download_handle->status = CORE_UPDATER_DOWNLOAD_ERROR;
+               break;
+            }
+#endif
+
+            /* Title that task_push_http_transfer_file() used to build
+             * internally; task_push_http_download_file() takes it as
+             * an argument instead. */
+            _tlen = 0;
+            http_title[0] = '\0';
+            strlcpy_append(http_title, sizeof(http_title), &_tlen,
+                  msg_hash_to_str(MSG_DOWNLOADING));
+            strlcpy_append(http_title, sizeof(http_title), &_tlen, ": ");
+            strlcpy_append(http_title, sizeof(http_title), &_tlen,
+                  transf->path);
+
             /* Push HTTP transfer task */
-            download_handle->http_task = (retro_task_t*)task_push_http_transfer_file(
-                  download_handle->remote_core_path, true, NULL,
-                  cb_http_task_core_updater_download, transf);
+            download_handle->http_task = (retro_task_t*)task_push_http_download_file(
+                  download_handle->remote_core_path, transf->path, true,
+                  http_title, cb_http_task_core_updater_download, transf);
 
             /* Update task title */
             task_free_title(task);

tasks/task_http.c

@@ -31,9 +31,7 @@ enum http_status_enum
 {
    HTTP_STATUS_CONNECTION_TRANSFER = 0,
    HTTP_STATUS_CONNECTION_TRANSFER_PARSE,
-   HTTP_STATUS_TRANSFER,
-   HTTP_STATUS_TRANSFER_PARSE,
-   HTTP_STATUS_TRANSFER_PARSE_FREE
+   HTTP_STATUS_TRANSFER
 };
 
 struct http_handle
@@ -54,7 +52,22 @@ struct http_handle
    bool error;
    bool headers_accept_err;
    bool sink_failed;
-   char connection_url[NAME_MAX_LENGTH];
+   /* Full request URL, owned.  This was a char[NAME_MAX_LENGTH]
+    * filled by strlcpy, but task_http_finder() compares the stored
+    * copy against the raw candidate URL -- so truncation on the
+    * stored side alone meant that past NAME_MAX_LENGTH (256, or 128
+    * on the small-path platforms) the two could never compare equal
+    * and the "concurrent download of the same file is not allowed"
+    * guard silently stopped guarding.  Playlist thumbnail URLs
+    * exceed it routinely, and the result was two tasks reaching
+    * cb_http_task_download_pl_thumbnail() and racing to
+    * filestream_write_file() the same path.
+    *
+    * NULL means "no key", which matches nothing: an allocation
+    * failure therefore degrades to admitting a redundant download
+    * rather than dropping one, which is the safe direction -- a
+    * dropped push strands the caller's completion callback forever. */
+   char *connection_url;
 };
 
 typedef struct http_handle http_handle_t;
@@ -124,8 +137,17 @@ static int cb_http_conn_default(void *data_, size_t len)
    if (!http)
       return -1;
 
+   /* Set http->error on this path too.  It used to just return -1,
+    * and the caller (task_http_conn_iterate_transfer_parse) discards
+    * the return value -- so the task advanced to HTTP_STATUS_TRANSFER
+    * with a NULL handle, net_http_update(NULL, ...) returned true
+    * immediately, and the task finished with neither data nor an
+    * error.  Callers saw a clean success with task_data == NULL. */
    if (!network_init())
+   {
+      http->error = true;
       return -1;
+   }
 
    if (!(http->handle = net_http_new(http->connection.handle)))
    {
@@ -192,8 +214,6 @@ static void task_http_transfer_handler(retro_task_t *task)
          if (!task_http_iterate_transfer(task))
             goto task_finished;
          break;
-      case HTTP_STATUS_TRANSFER_PARSE:
-         goto task_finished;
       default:
          break;
    }
@@ -273,6 +293,7 @@ task_finished:
       task_set_error(task, strldup("Internal error.",
                sizeof("Internal error.")));
 
+   free(http->connection_url);
    free(http);
 }
 
@@ -293,7 +314,8 @@ static bool task_http_finder(retro_task_t *task, void *user_data)
    http_handle_t *http = NULL;
    if (task && (task->handler == task_http_transfer_handler) && user_data)
       if ((http = (http_handle_t*)task->state))
-         return string_is_equal(http->connection_url, (const char*)user_data);
+         return http->connection_url
+             && string_is_equal(http->connection_url, (const char*)user_data);
    return false;
 }
 
@@ -350,9 +372,7 @@ static void *task_push_http_transfer_generic_titled(
    http->sink_file           = NULL;
    http->sink_path           = NULL;
    http->sink_failed         = false;
-   http->connection_url[0]   = '\0';
-
-   strlcpy(http->connection_url, url, sizeof(http->connection_url));
+   http->connection_url      = strdup(url);
 
    /* Streaming to disk: open the output now, so a bad path fails the
     * push rather than surfacing megabytes later, and arm the sink
@@ -406,6 +426,7 @@ error:
    if (http)
    {
       task_http_sink_close(http, false);
+      free(http->connection_url);
       free(http);
    }
 

tasks/task_http_emscripten.c

@@ -69,7 +69,13 @@ struct http_handle
     * data == NULL in the callback. */
    char                 *sink_path;
    bool                  settled;
-   char                  connection_url[NAME_MAX_LENGTH];
+   /* Full request URL, owned; see the matching comment in
+    * tasks/task_http.c.  A fixed buffer truncated the stored copy but
+    * not the candidate task_http_finder() compares it against, so the
+    * duplicate-download guard stopped working past NAME_MAX_LENGTH.
+    * NULL matches nothing, so OOM admits a redundant download rather
+    * than dropping one. */
+   char                 *connection_url;
 };
 
 typedef struct http_handle http_handle_t;
@@ -262,6 +268,7 @@ static void http_handle_free(http_handle_t *http)
    http_req_headers_free(http->req_headers);
    free(http->req_data);
    free(http->sink_path);
+   free(http->connection_url);
    free(http);
 }
 
@@ -318,7 +325,8 @@ static bool task_http_finder(retro_task_t *task, void *user_data)
    http_handle_t *http = NULL;
    if (task && (task->handler == task_http_transfer_handler) && user_data)
       if ((http = (http_handle_t*)task->state))
-         return string_is_equal(http->connection_url, (const char*)user_data);
+         return http->connection_url
+             && string_is_equal(http->connection_url, (const char*)user_data);
    return false;
 }
 
@@ -471,7 +479,7 @@ static void *task_push_http_transfer_generic(
    if (!(http = (http_handle_t*)calloc(1, sizeof(*http))))
       return NULL;
 
-   strlcpy(http->connection_url, url, sizeof(http->connection_url));
+   http->connection_url = strdup(url);
 
    /* Own a copy of the request body.  emscripten_fetch keeps the
     * pointer rather than copying it, so a caller's stack buffer, or