Skip to content

Commit 1759e1b

Browse files
committed
net/http: move postdata ownership to http_t instead of copying it in net_http_new
net_http_new(conn) used to malloc + memcpy conn->postdata into a fresh state->request.postdata buffer. For multi-MB POST bodies (file uploads, netplay syncs, translation-service requests) that is a full body-sized allocation + copy on every request dispatch. The copy was unnecessary. conn is freed by the caller right after net_http_new returns - both call sites in the tree follow the pattern conn = net_http_connection_new(...); /* optionally net_http_connection_set_content(conn, ...) */ h = net_http_new(conn); /* ... drive h to completion ... */ net_http_delete(h); net_http_connection_free(conn); (see tasks/task_http.c:90 and libretro-common/samples/net/net_http_test.c:66) and conn->postdata is only read by net_http_new and freed by net_http_connection_free - no other code path observes or uses it. The http_connection_t struct is forward-declared in the public header, so external code cannot reach postdata by any other route. Move ownership: assign state->request.postdata = conn->postdata, then null conn->postdata and zero conn->contentlength so net_http_connection_free does not double-free. One tracked allocation flows from net_http_connection_new / net_http_connection_set_content straight through to net_http_delete, with no intermediate copy. This also removes an OOM failure mode: the previous code's malloc could fail for large bodies on memory-constrained systems, and the caller had to detect that via the (conn->postdata && conn->contentlength && !state->request.postdata) check further down. With ownership transfer the setup cannot fail on the postdata path, so drop that OOM check and document why. Safety analysis: - Threading: conn is single-owner during net_http_new (no concurrent access; task_http.c's cb_http_conn_default is the only dispatcher and holds conn exclusively for the duration of the call). - Double-free: conn->postdata is nulled before net_http_new returns, so net_http_connection_free's 'if (conn->postdata) free(...)' is a no-op on the stolen field. - OOM during the rest of net_http_new: the postdata is already transferred by that point, so if any subsequent strdup fails and we take the error path, net_http_delete(state) frees the stolen postdata via its normal request.postdata teardown. No leak on the error path. - conn->contentlength is also zeroed; net_http_connection_free does not look at it so this is purely for consistency, but it also means anyone who later queries the emptied conn sees a correctly empty body rather than a claim of nonzero content-length with a NULL buffer. contenttype / domain / path / method / useragent / headers still use strdup - those are small and bounded, and keeping them as copies preserves the property that conn is still 'complete' after net_http_new for debugging or future reuse scenarios. Only postdata, which dominates the memory cost, gets the ownership-transfer treatment.
1 parent f3b3d69 commit 1759e1b

1 file changed

Lines changed: 20 additions & 5 deletions

File tree

libretro-common/net/net_http.c

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -837,10 +837,20 @@ struct http_t *net_http_new(struct http_connection_t *conn)
837837
state->request.contentlength = conn->contentlength;
838838
if (conn->postdata && conn->contentlength)
839839
{
840-
state->request.postdata = malloc(conn->contentlength);
841-
if (state->request.postdata)
842-
memcpy(state->request.postdata, conn->postdata, conn->contentlength);
843-
/* If malloc failed, the OOM guard further below catches it. */
840+
/* Move ownership of postdata from conn to state->request rather
841+
* than malloc+memcpy. conn is freed by the caller shortly after
842+
* this function returns (see task_http.c and the sample in
843+
* libretro-common/samples/net/net_http_test.c; both use conn
844+
* once with net_http_new then call net_http_connection_free),
845+
* and conn->postdata is only read by net_http_new and freed by
846+
* net_http_connection_free - no other code paths observe it.
847+
* Null the conn fields so net_http_connection_free does not
848+
* double-free. Eliminates an O(body size) copy that materially
849+
* matters for multi-MB POST payloads (file uploads, netplay,
850+
* translation service requests). */
851+
state->request.postdata = conn->postdata;
852+
conn->postdata = NULL;
853+
conn->contentlength = 0;
844854
}
845855
state->request.useragent= conn->useragent ? strdup(conn->useragent) : NULL;
846856
state->request.headers = conn->headers ? strdup(conn->headers) : NULL;
@@ -870,10 +880,15 @@ struct http_t *net_http_new(struct http_connection_t *conn)
870880
|| !state->request.path
871881
|| !state->request.method
872882
|| (conn->contenttype && !state->request.contenttype)
873-
|| (conn->postdata && conn->contentlength && !state->request.postdata)
874883
|| (conn->useragent && !state->request.useragent)
875884
|| (conn->headers && !state->request.headers))
876885
{
886+
/* Note: no postdata OOM check here. Ownership of postdata is
887+
* moved from conn (above), not copied, so the transfer cannot
888+
* fail. Both conn->postdata and state->request.postdata are
889+
* correctly set (NULL on conn, the original pointer on
890+
* state->request) regardless of any OOM elsewhere in this
891+
* function. */
877892
if (state->response.data)
878893
free(state->response.data);
879894
if (state->response.headers)

0 commit comments

Comments
 (0)