test(#715): behavioral coverage for copyctopstring deep-stack defenses#717
Merged
Merged
Conversation
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>
Owner
Author
/gate Review @
|
Owner
Author
/auto SummaryWhat went well: Plan agent's reachability audit was sharper than #715's original framing -- found that only 2 of the 5 deep-stack callsites have a live caller path, the other 3 are pure defense-in-depth pre-guarded upstream. Test design improved on PR #714's reference test in two concrete ways: tighter regex (full distinguishing phrase, no bare What could improve: The original issue body assumed "5 callsites need behavioral coverage." Reality was "2 reachable, 3 unreachable from any live caller." When filing follow-up issues from PR review, it's worth a quick Recommendations: None. The honest "2 reachable, 3 defense-in-depth" pattern in the test header is the right shape for documenting future audit gaps -- worth replicating if similar audits surface unreachable callsites elsewhere. 🤖 Generated with Claude Code /auto |
5 tasks
jsavin
added a commit
that referenced
this pull request
Jun 6, 2026
…laundering (#718) * fix(#716): parseerror signature now const char *; drop Pascal-string laundering Issue #716 item 1: langerror.c:167 cast a bigstring parameter to const char * before passing it to copyctopstring. The only caller (yyerror in langparser.c) was casting its actual const char * to (ptrstring) to match the dishonest signature. Two no-op casts laundering the real type through a Pascal-typed parameter for no reason. Fix shape: - parseerror takes const char * (its real type), matching the portable stub (runtime_stubs_parser.c) and yyerror's own argument - drop the (ptrstring) cast at yyerror's only invocation in both langparser.y and the checked-in bison-generated langparser.c - check copyctopstring's boolean return (post-#707) and log a warning on truncation so long bison syntax-error messages are observable rather than silently clipped Adjacent audit: langerror.c contains no other bigstring/cstring cast patterns. rg "copyctopstring\(\(const char \*\)" across Common/source, Common/headers, portable, frontier-cli, and tests now returns zero hits. Scope: this addresses only item 1 of issue #716. Items 2-6 (strings.c push-after-copy, shellsysverbs.c getenv, langsqlite errmsg, tcpverbs INDIRECT sites, db.compactDatabase YAML coverage) are separate defects and remain open. Tests: - new tests/parser_tests.c cases drive the yyerror -> parseerror chain: short message, long-token parse error, and direct parseerror call with a >255-byte C string. parser_tests now 12/12 (was 9). Direct run confirms the truncation warning fires on the >255-byte input. - make build: clean (6 pre-existing warnings, none new) - make unit: 589/589 pass (was 586; +3 new) - make test-integration: 2186 pass / 21 baseline failures (matches pre-change baseline per PR #717 commit log; no parse/langerror failures in the diff) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#716): address /gate P2 — include bounded prefix in truncation log; comment style Bar-raiser P2 #1: truncation log_warn omitted the prefix of the message that was truncated, limiting diagnostic value. Add bounded `%.80s...` so operators can see what the bison error looked like without unbounded log output. Bar-raiser P2 #3: switch the introductory block comment from leading-stars style to the date-prefixed plain block style used by the rest of langerror.c. P2 #2 (test names "does_not_crash" undersell what is asserted) declined: consistent with existing style in tests/parser_tests.c. Verified: make build clean, ./tests/parser_tests 12/12, full unit 589/589. Manual run confirms the new log line: [parse-WARN] langerror.c:183: parseerror: yacc message exceeded 255 bytes and was truncated: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx... Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced Jun 6, 2026
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 was referenced Jun 6, 2026
Merged
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 #715. Adds behavioral test coverage for the deep-stack
copyctopstringtruncation defenses landed in PR #714 (#712).PR #714 introduced 5 deep-stack defenses but the only behavioral test was for the CLI fast-fail at
main.c:660. This PR adds tests for the deep-stack callsites that are actually reachable from a live caller path.Tests added
tests/integration/cli_system_root_long_path_test.sh(4/4 GREEN)Covers
db_format.c:2388(migrate_internalsource path). Reaches the deep-stack callsite via--system-root <long-v6-path>— note this is a different CLI arg from--migrateand does NOT have the fast-fail check. Asserts:migrate_internal: source path exceeds 255 bytes and would be truncated.v7.tmpleakedtests/integration/cli_system_root_temp_path_boundary_test.sh(5/5 GREEN)Covers
db_format.c:2511(migrate_internaltemp.v7.tmppath). Boundary case: constructs a path in [248, 255] so the src path passes callsite Fix headless runtime warnings #2 butsrc + ".v7.tmp"(adds 7 bytes) trips callsite docs: UTF-8 transition plan #3. Asserts:migrate_internal: temp path exceeds 255 bytes and would be truncated.v7.tmpleakedNot behaviorally tested (documented in test header, intentional)
The Plan agent's audit found that 3 of the 5 deep-stack callsites have no live caller path:
db_format.c:2151(db_format_compact_to_pathdst)db.compactDatabaseverb has its OWN length guard atdbverbs.c:1232that fires before reaching this checkdbverbs.c:768(dbopenverbpost-migration reopen)filespectopathupstream (caps at 255)dbverbs.c:1467(db_migrate_reopen_if_legacy)filespectopathThey remain valid defense-in-depth (catches a future regression that removes the upstream guard) but are NOT reachable from any current external caller on macOS APFS. Faking the upstream-guard removal in a test would be a source-inspection test, not a behavioral one.
This is sharper than #715's original framing ("5 callsites need coverage"). The honest answer is "2 callsites are live-reachable; the other 3 are pure defense-in-depth without a live caller."
Test plan
./tests/integration/cli_system_root_long_path_test.shdirect invocation — 4/4 GREEN./tests/integration/cli_system_root_temp_path_boundary_test.shdirect invocation — 5/5 GREENcd tests && make test-system-root-long-path— passescd tests && make test-system-root-temp-path-boundary— passesNo production code changes. PR scope is tests-only.
🤖 Generated with Claude Code /auto