fix(pipeline): a bare Go reference never binds a struct Field - #1944
Conversation
USAGE, WRITES and READS edges resolve through the same short-name registry as CALLS but never consulted the DeusData#725 cross-language guard. On a Go tree with eBPF C probes every Go identifier spelled like a C one produced a reference edge into the C file: 31.5% of all WRITES on the originally measured repo crossed the Go->C boundary, led by dozens of Go test locals named event writing a C probe's automatic variable. Add cbm_suppress_cross_language_ref() - the reference-edge analog of cbm_suppress_cross_language_suffix_match - and consult it on BOTH resolvers of each edge type: the sequential pass (pass_usages.c resolve_usage_edges registry-fallback branch, resolve_rw_edges) and their parallel twins (pass_parallel.c resolve_file_usages, resolve_file_rw). The sequential-only version of this change left 344 Go->C WRITES alive on a ~1150-file repo because large repos resolve through pass_parallel.c - the field census caught it, and the parallel-twin test now pins it. Unlike the CALLS guard the predicate takes no strategy parameter: a reference edge carries no import-closure evidence, so every registry strategy is a bare-name guess across a boundary. LSP-backed semantic references resolve before the fallback and are unaffected. JS/TS stay one family, and C/C++ count as one family too (.h maps to CBM_LANG_CPP, so a .c file referencing its own header is not a boundary). Field-validated on the ~1150-file Go+C repo: Go->C/C++ WRITES 835->0, USAGE 1545->0; C->Go 15/90->0; 6432 reference edges dropped in total, every one cross-language (Go->.json 3034, Go->.hpp/.h 1612, Go->.c 746, Go->.sh/.yaml/.yml 443, ...) and none same-language. CALLS and IMPORTS totals are byte-identical to main. Fixes DeusData#1928 Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
Reproduce-first probe for the two fixes on this branch: RED without the descend fix (count_defs_with_label(r, "Field") == 0, expected 3), GREEN with it. Asserts the three named fields with their declared types in return_type, the absence of the blank identifier, and that interface members keep extracting exactly as before. Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
|
CI retrigger (no-op amend, identical tree — no rerun rights): |
37c0ef4 to
c175ca1
Compare
|
CI retrigger (no-op amend): |
c175ca1 to
7297139
Compare
The READS/WRITES resolvers and the USAGE registry fallback hand bare reference text to the short-name registry, which contains Field nodes - so once Go struct fields exist (DeusData#1935), every Go local err := ... binds whichever struct field is named err, project-wide: 21308 USAGE and 5191 WRITES onto Go fields on the measured repo, top target a test struct's field T collecting 3013 edges. In Go that binding is impossible by construction: a field is only reachable through a selector expression (x.f), and selector references resolve on the LSP path - every Field-targeted reference edge in the census carried dot-less text. Add cbm_go_suppress_bare_field_ref() next to the DeusData#1928 predicate and consult it at the same four sites (both READS/WRITES resolvers, both USAGE registry fallbacks): drop the bind when the file is Go, the target label is Field, and the reference text has no '.'. Go-gated because C#/Java/C++/Python method bodies legitimately reference their own members bare (cp_reads_writes_cs_static_field pins that shape). Field-validated on the DeusData#1940 stack: USAGE onto Go fields 21308 -> 0, WRITES 5191 -> 0; the only remaining field-targeted edges are 2466 CALLS, which are DeusData#1906/DeusData#1907's selector-guard territory. Reproduce- first pipeline probes (sequential + parallel twins) were RED on the stack without this commit. Fixes DeusData#1942 Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
|
Fresh-day CI retrigger (no-op amend): third consecutive |
7297139 to
cc00a02
Compare
|
#1937 and #1940 are merged — First, the thing you should not have had to do: stop force-pushing to chase On the review itself — this is unusually careful work, and the specific things that made it reviewable are worth naming, because they are the things that usually go missing:
I verified the load-bearing claim in #1937 by reading the predicate rather than trusting the description: it fails open on unknown caller language, missing or empty path, unknown target language, same language, the JS/TS family, and the C/C++ family. It only drops when both languages are known and differ across families. That is the property that makes it a correctness fix rather than a recall trade, and it is why it could merge without a measurement argument. The stack order you specified was also correct and load-bearing, so it was followed exactly: #1937 first, then #1940. Landing the ~4600 Thank you — this is a strong first contribution to the project. |
|
Post-merge correction, because a review pass after the merge found something I should have caught before it: the Both call paths strip the receiver before the predicate ever sees the name:
So the guard is a blanket veto on Field-targeted references rather than the precise one it documents. Two consequences worth stating plainly, since they bear on evidence I accepted at merge time:
None of this is a regression — The signal the guard actually wants — No action needed from you unless you want to take #1962 — and no criticism intended in any of it. You disclosed the collision class this stack creates before anyone asked, which is why the follow-up was findable at all. |
|
Thank you for the merge review — and especially for the post-merge correction. You are right on both counts: the extractor strips the receiver on every path that reaches the resolver, so the dot test cannot fire, and the census I presented was measuring the extractor's contract back at itself. I should have traced the ref_name provenance before claiming the 100% figure as evidence. Taking #1962: the plan is exactly the signal you point at — record is-member-access at extraction (the field_identifier half of a selector) on CBMUsage/CBMReadWrite, and let the guard consume that instead of string shape, so genuine |
… not reference text cbm_go_suppress_bare_field_ref dropped a Field-targeted reference when strchr(ref_name, '.') == NULL — but the extractor strips the receiver on every path that reaches the resolver (resolve_lhs_write_name records the trailing field name of a selector LHS; is_reference_node records the inner field_identifier), so the dot test could never be false for Go. The guard was a blanket veto: all ~4588 Go Field nodes recovered by one 61de19b were unreachable by USAGE/READS/WRITES, including genuine selector references (issue #1962, found in maintainer post-merge review of #1944). The selector-vs-bare distinction exists at extraction time and was discarded; record it and let the resolver consume it: - CBMUsage/CBMReadWrite gain is_member_access. The usage recorder sets it when the reference node is a field_identifier (the member half of a selector); resolve_lhs_write_name reports it through an out-param when it takes the field/member LHS branch. - cbm_go_suppress_bare_field_ref(is_go, is_member_access, target_label) refuses a Field bind only for references that were never the member half of a selector. All four resolver sites (sequential and parallel, USAGE and READS/WRITES) pass the recorded flag. Reproduce-first: with the veto restored, the extended #1942 fixtures go RED on both resolver paths at the new asserts (t.err = nil must WRITE the field; t.n must produce USAGE); with the fix both twins are GREEN and the original bare-local negatives still hold. The dead ASSERT_FALSE(..., "t.err", ...) unit case — an input Go's extractor cannot produce — is replaced by flag-based cases. Fixes #1962. Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
2 participants
Fixes #1942. Stacked on #1937 → #1940 — the reproduce-first probe needs Go
Fieldnodes to exist (#1940) before a bare local can falsely bind one, and #1940 needs this before its node recovery is net-positive for reference edges. The tip commit is this PR's change; suggested merge order #1937 → #1940 → this (or the last two together).What
The READS/WRITES resolvers and the USAGE registry fallback hand bare reference text to the short-name registry, which contains
Fieldnodes — so once Go struct fields exist, every Go localerr := …binds whichever struct field is namederr, project-wide:Fieldnodes (stack without this fix)USAGET, 3013 edges)WRITESIn Go the binding is impossible by construction: a field is only reachable through a selector expression (
x.f), and selector references resolve on the LSP path — 100% of the Field-targeted USAGE edges in the census carried dot-less reference text, so there is no legitimate population mixed in.How
cbm_go_suppress_bare_field_ref(is_go, ref_name, target_label)— pure predicate next to #1937's, consulted at the same four sites (both READS/WRITES resolvers, both USAGE registry fallbacks): drop when the file is Go, the target label isField, and the reference has no.. Go-gated: C#/Java/C++/Python method bodies legitimately reference their own members bare —cp_reads_writes_cs_static_fieldpins that shape as required, and it stays untouched.Field-validated (fresh index, ~1150-file Go+C repo, full stack)
USAGE onto Go fields 21308 → 0, WRITES 5191 → 0. The only remaining field-targeted reference class is 2466
CALLS, which is #1906/#1907's selector-guard territory. Combined stack ends below main's reference totals (USAGE 29198 → 21035, WRITES 2681 → 774) while adding the 4533 field nodes.Tests
pipeline_go_bare_ref_never_binds_field+_paralleltwin (both resolvers, per fix(pipeline): guard USAGE/WRITES/READS against cross-language binds #1937's lesson): RED on the fix(extract): descend into Go struct field_declaration_list #1940 stack (ASSERT(!(cross_file_edge_exists(s, project, "Run", "err", "WRITES")))failed on both paths), GREEN with the predicate. Fixture asserts the field node exists, so the probe can't pass vacuously.go_bare_ref_never_binds_field: selector-shaped refs may bind, non-Field targets untouched, non-Go languages untouched, degenerate inputs safe.scripts/test.shvenue leg (ASan+UBSan, all suites + contract steps): green on the stack.git clang-format --diff: clean.#1932 tracks the family. Same root class as #1927: a name-shape heuristic binding without checking what the name can denote.