fix(#712): surface copyctopstring truncation at user-facing callsites#714
Merged
Conversation
- frontier-cli/main.c: fail-fast check rejects --migrate paths > 255 bytes before any resource allocation, with a clear "exceeds 255 bytes" message. This turns the test from 3/4 to 4/4 green. - Common/source/db_format.c (migrate_internal, line ~2378): check return of copyctopstring for db_path (source path); goto cleanup with descriptive fail_step on truncation. - Common/source/db_format.c (migrate_internal, line ~2501): same check for temp_path (output + ".v7.tmp" suffix -- always longer than db_path). - Common/source/db_format.c (db_format_compact_to_path, line ~2151): same check for dst_path supplied by db.compactDatabase UserTalk verb. - Common/source/dbverbs.c (~768): check copyctopstring return on migrated output_path in dbopenverb; fail with log_error before pathtofilespec sees a truncated path. - Common/source/dbverbs.c (~1467): same in db_migrate_reopen_if_legacy. - portable/fileverbs_portable.c (~520): comment-only documenting intentional truncation-swallow in the error-message formatter on the error path already. All 586 unit tests pass. Integration suite: 2186/2414 pass, 21 baseline failures unchanged (documented in PR #708 gate review). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Asserts --migrate rejects >255-byte paths with a clear truncation error rather than the generic openfile(src) failure. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
/gate Review @
|
| # | Sev | File:Line | Finding | Action |
|---|---|---|---|---|
| 1 | P2 | tests/Makefile:694-695 |
Stale RED-state comment claims test fails today (it now passes) | Will fix in this PR |
| 2 | P2 | frontier-cli/main.c:655 |
#define MIGRATE_MAX_PATH_BYTES 255 inside function body — legal but unusual style |
Hoist to top-of-file in this PR |
| 3 | P2 | Test coverage gap | Integration test covers only CLI fast-fail at main.c:656. Deep-stack checks at db_format.c (3 sites) and dbverbs.c (2 sites) lack behavioral coverage. |
File follow-up issue for db.compactDatabase YAML test |
| 4 | P2 | tests/integration/cli_migrate_long_path_test.sh:148 |
Bare truncat token would also match "untruncated" / "truncating" |
Tighten regex in this PR |
Security praise
- Defense-in-depth pattern is correctly placed at every reachable callsite
- All five new
copyctopstringchecks fire BEFORE the consumingpathtofilespec/openfilecall - UTF-8 mid-codepoint truncation concern is moot: the new check rejects the path entirely before
pathtofilespecever sees it migrate_internalcleanup is safe when the firstcopyctopstringfails (bigstrings stack-allocated, no handles allocated yet,temp_pathnot yet a real file)db_format_compact_to_pathcallers propagate the boolean tolangerrormessage(dbverbs.c:1346)- Integration test is behavioral (not source-grep): builds a real 449-byte path, copies real v6 fixture, invokes real CLI, asserts no
.v7.tmpleak and v6 source unchanged
Bar-raiser praise
- No "while I'm here" creep; every diff is on-thesis
- CLI pre-check boundary math is correct (
> 255, matchescopyctopstring'ssrclen <= 255accept condition) db_format.cadditions match file's pre-existing space indentation;dbverbs.cmatches file's pre-existing tab indentation (per CLAUDE.md "mass retab tracked separately")clang-tidy "sizeof(array) > 0"tautology atdb_format.c:2379is pre-existing (commitabb4634925), NOT introduced by this PR
Concurrency praise
- CLI pre-check at
main.c:656runs beforeheadless_threading_init()— no threads exist yet - New checks in
db_format.cmigrate_internalanddbverbs.ccontain nolangbackgroundtask/ yield points last_migration_output_pathis_Thread_local— no shared-state racefail_stepis stack-local; no thread can observe between set andcleanup:since GIL is held throughout
Branch will be ready to merge after the 4 P2 fixes land.
- Hoist MIGRATE_MAX_PATH_BYTES to top-of-file in frontier-cli/main.c (was function-local; legal but unusual). Add header comment explaining the 255-byte limit and the CLI-vs-deep-stack defense layering. - Update tests/Makefile comment for test-migrate-long-path (was stale RED-state description; test is now GREEN post-fix). - Tighten test regex from bare `truncat` to `truncat(ed|ion|ing)` to avoid matching unrelated tokens like "untruncated". Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
/gate Round-2 Review @
|
| # | Round 1 P2 | Status |
|---|---|---|
| 1 | Stale RED-state comment in tests/Makefile:691-695 |
FIXED — describes post-fix steady state |
| 2 | #define MIGRATE_MAX_PATH_BYTES mid-function |
FIXED — hoisted to file scope alongside SYSTEM_ROOT_FILENAME, in-function comment cross-references it |
| 3 | Test coverage gap (deep-stack defenses unexercised) | DEFERRED to follow-up issue (acceptable — CLI fast-fail is user-facing path; deep-stack needs different harness) |
| 4 | Bare truncat token in test regex |
FIXED — tightened to truncat(ed|ion|ing) |
No "while I'm here" creep; diff is exactly the 4 nit fixes (3 files, 15 lines).
Branch is ready to merge.
3 tasks
Owner
Author
/auto SummaryWhat went well: Plan agent correctly classified the 338 What could improve: The integration test only behaviorally covers the CLI fast-fail. The deep-stack defenses at Recommendations: When the followup #715 lands, consider promoting the path-construction logic (449-byte nested-directory path with cleanup trap) from the shell test into a reusable test helper so the C-side fixtures don't reinvent it. Also worth noting: the audit surfaced 6 separate defects worth their own issues (langerror.c:167 cast-from-bigstring UB, strings.c:1315/1319 push-after-copy truncation, shellsysverbs.c:594 unbounded getenv, langsqlite errmsg sites, tcpverbs INDIRECT sites, db.compactDatabase verb-side YAML test) — track these so the audit-tail doesn't get lost. 🤖 Generated with Claude Code /auto |
This was referenced Jun 5, 2026
jsavin
added a commit
that referenced
this pull request
Jun 5, 2026
#717) PR #714 (#712) added 5 deep-stack defenses; only the CLI fast-fail at main.c:660 had behavioral coverage. This PR adds two shell integration tests for the deep-stack callsites reachable from live callers: - cli_system_root_long_path_test.sh: exercises db_format.c:2388 (migrate_internal src) via --system-root with a long v6 path, which bypasses the --migrate CLI fast-fail. 4/4 GREEN. - cli_system_root_temp_path_boundary_test.sh: exercises db_format.c:2511 (migrate_internal temp_path) by constructing a path in [248, 255] so the src passes but src + .v7.tmp overflows. 5/5 GREEN. Callsites #1 (db_format_compact_to_path), #4 and #5 (dbverbs.c) are documented in the test header as not behaviorally reachable: #1 is pre-guarded by db.compactDatabase verb-level length check at dbverbs.c:1232; #4 and #5 receive paths pre-clamped by filespectopath. They remain valid defense-in-depth without a live caller path. No production code changes. Baselines preserved: unit 586/586, integration 2186 pass / 21 baseline failures. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
4 tasks
jsavin
added a commit
that referenced
this pull request
Jun 6, 2026
…720) * fix(#716): item 5 -- bound >255-byte hostname in tcp_address_to_name NI_MAXHOST is 1025 (macOS, Linux) to accommodate non-DNS resolution sources (NIS, LDAP, /etc/hosts), but UserTalk's bigstring max payload is 255. A long PTR record -- or an attacker-controlled reverse lookup that returns a crafted hostname like "evil.com.<padding>...<more padding>.victim.com" -- would silently truncate via copyctopstring's post-#707 clamp, potentially leaving the prefix "evil.com" visible to the UserTalk-level identity check while concealing the actual intent of the lookup. The fix surfaces the truncation as an explicit log_warn and falls back to the dotted-decimal IP string -- the same fallback path used when getnameinfo() itself fails. Pattern matches frontier-cli/window_registry.c:116 and PR #714's post-#707 callsites. For testability, the hostname-to-bigstring decision was extracted from tcp_address_to_name into a new helper tcp_address_to_name_pack. The helper takes the resolved C string and the original address, so the truncation branch is testable without mocking getnameinfo. Two new behavioral unit tests in tests/tcp_phase1a_unit_tests.c: - address_to_name_pack_long_hostname_falls_back_to_ip: 300-byte synthetic hostname -> helper returns false, name_out is "192.168.1.1" - address_to_name_pack_short_hostname_preserved: "example.com" -> helper returns true, name_out is "example.com" verbatim Other tcpverbs.c copyctopstring callsites audited and verified bounded by construction: - tcp_set_error (line 387): char[256] bounded by snprintf - tcp_address_decode (line 666): INET_ADDRSTRLEN (16 bytes) - 9 string-literal sites (INACTIVE/OPEN/DATA/STOPPED/LISTENING/etc.) with payloads under 10 bytes WinSockNetEvents.c is Windows-only, not linked into headless build. Verified: - make build: clean - ./tools/run_headless_tests.sh: 591/591 pass (was 589, +2 new) - cd tests && make test-integration: 21 baseline failures (matches PR #717 baseline; no tcp_address_to_name failures) Closes item 5 of #716. Items 1, 3, 6 already closed; items 2, 4 closed as documentation-only. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#716): address /gate P2 -- rationale comments + test memset; file #721 Security reviewer P2: add inline rationale to tcp_address_to_name_pack explaining (a) why dotted-decimal IP is the right fallback rather than empty / langerrormessage (UserTalk-caller identity-check safety, contract preservation), and (b) why the log_warn intentionally omits the truncated hostname (attacker-controlled, log-injection risk). Security reviewer P2: memset(name_out, 0, sizeof(name_out)) before each test call so a future regression that early-returns without writing surfaces as a deterministic empty string rather than reading stack garbage. Bar-raiser P2: WinSockNetEvents.c:1419 has the same bug pattern in the Windows path (not in headless build). Filed as #721 to track when/if the Windows path is revived. Bar-raiser P2 (comment placement) declined: the doc comment for tcp_address_to_name still reads correctly as "see the helper above for the truncation contract" -- moving it would create a different clarity tradeoff. Tests still 33/33. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <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
1 participant
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.
Summary
Closes #712. Defense-in-depth follow-up to PR #708 (the canonical
copyctopstringbuffer-overflow fix).PR #708 made
copyctopstringreturnboolean(false on truncation). This PR adds explicit checks at the user/filesystem-derived callsites so >255-byte input surfaces as a clear, actionable error instead of silently truncating.Callsites fixed
Common/source/db_format.c:2151(db_format_compact_to_pathdst path)Common/source/db_format.c:2378(migrate_internalsource path)Common/source/db_format.c:2493(migrate_internaltemp .v7.tmp path)Common/source/dbverbs.c:768(dbopenverbpost-migration reopen path)Common/source/dbverbs.c:1467(db_migrate_reopen_if_legacy)portable/fileverbs_portable.c:520(comment-only -- the truncation is in an error-message formatter already on the error path; documented as intentional)CLI fast-fail
frontier-cli/main.c-- newMIGRATE_MAX_PATH_BYTES(255) check at argv-parse time rejects oversized--migratepaths before any resource allocation, with:The deep-stack checks in
db_format.c/dbverbs.care defense-in-depth: they catch paths that arrive throughdb.open()auto-migration or the direct C API, not just the CLI.Test plan
tests/integration/cli_migrate_long_path_test.sh-- constructs a 449-byte path, asserts non-zero exit, asserts stderr identifies path-length as the cause, asserts no.v7.tmpleaks at the truncated prefix, asserts the v6 source file is untouched. Was RED before fix (test went red on the "stderr identifies path-length" assertion because the user saw a genericmigrate fail at openfile(src)instead); now 4/4 GREEN.Deferred (out of scope -- noted by Plan agent for follow-up)
The Plan agent's audit surfaced several additional
copyctopstringcallsites that warrant separate issues, intentionally NOT addressed here to keep this PR scoped:repl.c,completion.c) -- defensive only.langsqlite.cerrmsg sites (cosmetic truncation of SQL error messages).tcpverbs.c,WinSockNetEvents.c,shellsysverbs.cINDIRECT callsites.langerror.c:167-- casts a bigstring (Pascal-encoded) toconst char *and passes it tocopyctopstring. The length byte is read as the first character; likely UB. Separate defect class.Common/source/strings.c:1315, 1319-- push-after-copyctopstring sites where the PUSH (not the initial copy) is the truncation surface. Different bug class.shellsysverbs.c:594(getenv) -- unbounded env-var input.db.compactDatabaseUserTalk verb -- behavioral analog of the--migratetest would be a YAML integration test callingdb.compactDatabase("<long path>").A separate issue can be filed to track the audit-tail.
🤖 Generated with Claude Code /auto