server: fix client-triggerable double-free in JSON request parsing#301
Open
ferdinandobons wants to merge 1 commit into
Open
server: fix client-triggerable double-free in JSON request parsing#301ferdinandobons wants to merge 1 commit into
ferdinandobons wants to merge 1 commit into
Conversation
The HTTP request parsers free a string field and then re-parse into the
same field, e.g.:
free(r->model);
if (!json_string(&p, &r->model)) { ...; goto bad; }
The readers (json_string, json_content, json_raw_value, parse_prompt,
parse_anthropic_system, parse_responses_content_array) never write *out on
their failure paths, so on a malformed value the field keeps pointing at the
just-freed buffer and the error path frees it again (request_free, or a
cleanup label such as bad:/done:/fail:/item_fail:, or tool_call_free) — a
double-free fully controlled by the request body.
For r->model the field is non-NULL from request_init(), so a single request
is enough, no duplicate key needed:
POST /v1/chat/completions {"model": 5}
POST /v1/messages {"model": 123}
POST /v1/responses, /v1/completions likewise
Every other free-then-reparse field is the same class, reachable via a
duplicated object key whose second value is malformed, e.g. a content block
{"type":"text","type":5} or tool_calls {"arguments":"x","arguments":"\u"}.
Fix: set the pointer to NULL immediately after the free at every
free-then-reparse site (48 sites), so a failed re-parse leaves it NULL and the
cleanup free() becomes a no-op. On success the field is overwritten by the
parsed value, so behaviour on valid input is unchanged.
Verified under AddressSanitizer with a harness that #includes ds4_server.c and
calls the real parsers: on the unpatched code {"model":5},
[{"function":{"arguments":"x","arguments":"\u"}}] and
{"prompt":"x","prompt":"\u"} each abort with "attempting double-free"; with
this change all three return cleanly with no double-free. Builds clean with
-Wall -Wextra.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fix a client-triggerable double-free in the HTTP JSON request parsers in
ds4_server.c.The request parsers free a string field and then re-parse into the same field:
The readers —
json_string,json_content,json_raw_value,parse_prompt,parse_anthropic_system,parse_responses_content_array— never write*outon their failure paths. So on a malformed value the field keeps pointing at the just-freed buffer, and the error path frees it again (request_free, a cleanup labelbad:/done:/fail:/item_fail:, ortool_call_free) → a double-free fully controlled by the request body.Impact
r->modelis initialized non-NULL byrequest_init(), so the double-free fires on the first malformed occurrence — no duplicate key needed:Every other free-then-reparse field is the same class, reachable via a duplicated object key whose second value is malformed, e.g. a content block
{"type":"text","type":5}or tool_calls{"arguments":"x","arguments":"\u"}.Fix
Set the pointer to
NULLimmediately after thefree()at every free-then-reparse site (48 sites). Safe and behaviour-preserving:*out, so the field staysNULLand the cleanupfree()is a no-op.Testing — AddressSanitizer
A harness that
#includesds4_server.cand calls the real parsers (no reimplementation). Three independent triggers, each hitting a different cleanup path:{"model": 5}parse_chat_request→request_free[{"function":{"arguments":"x","arguments":"\u"}}]parse_function_call→tool_call_free{"prompt":"x","prompt":"\u"}parse_completion_requestcleanupmain): all three abort withAddressSanitizer: attempting double-free.invalid JSON request), no double-free.-O3 -ffast-math -Wall -Wextra -std=c99); a completeness scan finds no remaining free-then-reparse site without a following= NULL.No functional change on valid requests.