Repository navigation
fix(file): reject reads that do not fit the result buffer - #120
Conversation
file_read returned success after clipping to the caller buffer. The agent passes a 4096-byte slot, so a later write_file could replace the file with that prefix and delete the rest. Supersedes #114.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Solid, focused fail-closed fix for a real silent data-loss bug: file_read no longer returns success with a clipped prefix that a later write_file could turn into a truncated file. Same pattern as session JSON (#70) and ASAP reply (#61) refuse-vs-truncate. Local ./build/test_file passed (83 tests, 0 failed; link with -lm when -lcurl is missing).
Must Fix
None.
Should Fix
- Add an Unreleased Fixed bullet in
CHANGELOG.mdfor this behavior (same gap as #119). Other high-severity refuse-vs-truncate fixes in this tree are changelogged. - In
tests/test_file.c,buf[32]is smaller than the error JSON ({"error":"file exceeds result buffer","bytes":64}is ~49 bytes), sosnprintftruncates the message and the assertions only prove a substring"error". Use a result buffer large enough for the full error (and still smaller than the 64-byte file), assert"bytes"/ the error string, and add a one-over case (size == max_lenfails,size == max_len - 1already covered).
Nice to Have
- Mention the effective ~4095-byte read cap (agent
TOOL_RESULT_SIZEis 4096) in the file tool description so the model does not retry blindly on large notes. - Offset/length parameters for large files are out of scope here; fail-closed is the right v1 tradeoff.
Positive Highlights
- Correct root-cause fix: reject when
to_read + 1 > max_len, keep exact-fit success, close the file before writing the error. - Regression test locks both oversize failure and exact-fit success; single-commit PR that supersedes #114 without extra scope.
Sent by Cursor Automation: Code review
| if (to_read + 1 > max_len) { | ||
| fclose(f); | ||
| snprintf(result_buf, max_len, | ||
| "{\"error\":\"file exceeds result buffer\",\"bytes\":%ld}", sz); | ||
| return -1; |
There was a problem hiding this comment.
Fail-closed here is the right contract: a successful read must be the whole file (agent slot is TOOL_RESULT_SIZE 4096 in agent.c). Exact fit (size == max_len - 1) still succeeds; size >= max_len correctly errors. Good match to the refuse-vs-truncate pattern used elsewhere.
| char buf[32]; | ||
| char exact[8]; | ||
| FILE *f; | ||
| config_t *cfg; | ||
| const tool_t *t; | ||
| int i; | ||
|
|
||
| snprintf(tmpdir, sizeof(tmpdir), "/tmp/sc_test_readcap_%d", (int)getpid()); | ||
| if (mkdir(tmpdir, 0755) != 0 && errno != EEXIST) | ||
| return; | ||
| snprintf(notes_path, sizeof(notes_path), "%s/notes.md", tmpdir); | ||
| f = fopen(notes_path, "w"); | ||
| MU_ASSERT(f != NULL, "create oversized notes.md"); | ||
| for (i = 0; i < 64; i++) | ||
| fputc('A', f); | ||
| fclose(f); | ||
| cfg = make_ws_config(tmpdir, config_path, sizeof(config_path)); | ||
| MU_ASSERT(cfg != NULL, "readcap: load config"); | ||
| tool_file_set_config(cfg); | ||
| t = tool_file_get(); | ||
| snprintf(args, sizeof(args), "{\"operation\":\"read_file\",\"path\":\"%s\"}", notes_path); | ||
| MU_ASSERT(t->execute(args, buf, sizeof(buf)) == -1, "oversize read fails"); | ||
| MU_ASSERT(strstr(buf, "error") != NULL, "oversize read reports an error"); | ||
| MU_ASSERT(strstr(buf, "AAAAAAAAAAAAAAAA") == NULL, "oversize read is not a raw prefix"); |
There was a problem hiding this comment.
Should Fix: buf is 32 bytes, but the error JSON is longer (~49 for bytes":64), so this path only checks a truncated prefix. Prefer something like char err[128] (still < 64 so the oversize path triggers) and assert strstr(buf, "file exceeds result buffer") and strstr(buf, "bytes"). Optionally add a size == sizeof(exact) one-over case next to the exact-fit success.
adriannoes
changed the title
Add the Unreleased Fixed bullet for the file read bounds fix so the behavior change is changelogged like the other refuse-vs-truncate fixes in this tree. Review of #120 flagged the missing entry; docs only, no behavior change.
The oversize regression used a 32-byte result buffer, smaller than the ~49-byte error JSON, so snprintf truncated the message and the assertions only proved a bare "error" substring. Give the oversize case a 56-byte buffer (still below the 64-byte fixture) so the exact refuse-to-truncate error and the "bytes" field can be asserted, and cover the last boundary: a read of exactly max_len bytes fails where max_len - 1 already succeeds. Test-only change per review of #120.
|
Review response — done in 0104890 and fd3a098: changelog bullet, and the oversize test now asserts the full error JSON (including "bytes") with a buffer that fits it, plus the size == max_len one-over case. Deferred: noting the ~4095-byte read cap in the tool description (model-facing prompt change). |
# Conflicts: # CHANGELOG.md
1 participant
Summary
Consolidates one cursor high-severity fix (originally
cursor/high-severity-issues-ed43, PR #114) as an operator-authored PR.file_read returned success after clipping to the caller buffer. The agent passes a 4096-byte slot, so a later write_file could replace the file with that prefix and delete the rest.
Testing
CI=true make test— all 45 test binaries passed (includes test_file)make static— passedSupersedes #114.