From 3566d23732c5b48ede600084a96319ecd4d9f43b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 20 Sep 2026 11:16:48 +0000 Subject: [PATCH] fix(asap): free inbound response payload once on envelope OOM fill_response_envelope attached the payload cJSON then deleted it and called asap_envelope_clear, which deleted the same object. Unauthenticated POST /asap state.query and task.cancel reach this path; required-field strdup failure is a heap double-free. Co-authored-by: esadrianno --- CHANGELOG.md | 1 + src/asap/server.c | 3 ++- tests/test_asap_server.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 901fb38..492bd9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ All notable changes to ShellClaw are documented here. Format follows [Keep a Cha ### Security - Camera auto-output keeps the exclusive `mkstemp` inode (no unlink + `${tmpl}.jpg` sibling). - Reject I2C `bus` outside 0–255 at the tool JSON boundary. +- Inbound ASAP response builder no longer double-frees the payload cJSON when a required envelope field cannot be allocated (unauthenticated `POST /asap` `state.query` / `task.cancel`). --- diff --git a/src/asap/server.c b/src/asap/server.c index d681f9b..d58ddcd 100644 --- a/src/asap/server.c +++ b/src/asap/server.c @@ -98,13 +98,14 @@ static int fill_response_envelope(asap_envelope_t *out, const asap_envelope_t *i out->sender = in->recipient ? strdup(in->recipient) : NULL; out->recipient = in->sender ? strdup(in->sender) : NULL; out->payload_type = strdup(payload_type); + /* Ownership of payload moves to out; asap_envelope_clear frees it once. */ out->payload = payload; + payload = NULL; if (in->correlation_id) out->correlation_id = strdup(in->correlation_id); if (in->trace_id) out->trace_id = strdup(in->trace_id); if (!out->id || !out->asap_version || !out->sender || !out->recipient || !out->payload_type) { - cJSON_Delete(payload); asap_envelope_clear(out); asap_envelope_init(out); return -32603; diff --git a/tests/test_asap_server.c b/tests/test_asap_server.c index 1669864..b07b59b 100644 --- a/tests/test_asap_server.c +++ b/tests/test_asap_server.c @@ -686,6 +686,38 @@ static int test_tool_execute_nonzero_reports_error(void) return 0; } +/* + * fill_response_envelope used to cJSON_Delete(payload) after assigning + * out->payload, then asap_envelope_clear(out) deleted the same object. + * HTTP parse always supplies sender/recipient, so production hits this on + * post-attach strdup OOM; dropping sender here is the same cleanup path. + */ +static int test_response_builder_missing_sender_does_not_double_free(void) +{ + asap_envelope_t in; + asap_envelope_t out; + asap_server_ctx_t ctx; + char err[128]; + cJSON *pl; + int rc; + + pl = cJSON_CreateObject(); + ASSERT(pl != NULL); + ASSERT(cJSON_AddNullToObject(pl, "task_id") != NULL); + ASSERT(wrap_build(&in, "task.cancel", pl) == 0); + free(in.sender); + in.sender = NULL; + memset(&ctx, 0, sizeof ctx); + asap_envelope_init(&out); + rc = asap_server_handle(&in, &out, &ctx, err, sizeof err); + ASSERT(rc == -32603); + ASSERT(strstr(err, "envelope") != NULL); + ASSERT(out.payload == NULL); + teardown_env(&in); + teardown_env(&out); + return 0; +} + static int test_trust_sender_rejects_blank_sender_when_list_nonempty(void) { const char *path = "/tmp/shellclaw_test_asap_trust_blank.toml"; @@ -747,5 +779,6 @@ int main(void) r |= test_tool_call_hook_overrides_builtin_dispatch(); r |= test_tool_execute_nonzero_reports_error(); r |= test_trust_sender_rejects_blank_sender_when_list_nonempty(); + r |= test_response_builder_missing_sender_does_not_double_free(); return r; }