crashappsec / crashappsec/ncc

lto_guard: extend to struct fields beyond top-level; add targeted analysis to minimize protected surface

Open
#7 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
0
Forks
0
Avg merge
5d 4h
Merged PRs (30d)
1

Description

The `--ncc-lto-guard` flag injects `volatile` on the rightmost `*` of every
block-scope local pointer declaration, and (as of the struct-field extension)
every pointer-typed `member_declaration` field. This defeats the
LTO/callee-saved-register aliasing bug where a conservative GC's stack scan
fails to see pointer locals that the compiler has hoisted into a callee-saved
register across a possible-GC call (or struct field that the GC has moved
forwarded but our register copy stayed stale).

Two follow-ups that came out of using this in n00b-regex:

## 1. Extend struct-field volatile beyond top-level

Today's member-declaration pass only qualifies pointers declared directly in
the `member_declarator`. It does **not** qualify:

- pointer-typed members of nested anonymous unions/structs accessed via
`outer.union_arm.ptr`
- pointer-typed members of arrays of structs (`struct S arr[N];` — the array
storage is fine, but `arr[i].p` accesses still need the field qualified,
which today they are, but verify behavior with VLA / flexible array)
- members declared via type aliases / typedefs that resolve to struct
pointers (e.g. `typedef Foo *FooP; struct { FooP p; };` — we'd need to
recognize `p` as a pointer through the typedef chain)

Today's pass also skips function-pointer struct members entirely. That's
correct for not corrupting the type, but it leaves a class of holders
unprotected (a function pointer assigned to a stale address would still
crash). If we treat function pointer fields as needing protection too,
we'd need to inject volatile in the `(*name)` slot rather than the outer
`*`.

## 2. Targeted analysis to minimize the protected surface

The current xform is blanket: every block-scope local pointer and every
pointer struct member gets `volatile`. This:

- generates `-Wincompatible-pointer-types-discards-qualifiers` warnings at
every `&local` or `&struct->field` that's passed to a function expecting
a non-volatile `T**` (common: `n00b_atomic_cas`, `pthread_cond_wait` style
out-params). Today these are warnings only, but they pollute build output
and signal a real semantic mismatch.
- prevents optimizations the compiler could otherwise apply for locals that
never escape a possible-GC call (e.g. a pointer only used between two
pure arithmetic ops on the stack).

A better xform would:

- track which function calls inside the current block can possibly trigger
a GC (transitively). Allocation calls (`n00b_alloc*`), explicit
`n00b_collect`, and any function not annotated as no-GC are candidates.
- only `volatile`-qualify pointers whose live range *crosses* such a call
AND whose address is not taken AND whose value escapes into a register
rather than always being spilled.
- for struct fields, qualify only members whose containing struct is
scanned by GC (i.e., not in a `hidden`/`scan_kind=NONE` pool) — pure POD
arenas would not need it.

This needs reaching-definitions or escape analysis over the parse tree.
ncc doesn't have a control-flow IR, so a conservative approximation would
be: any pointer local declared above a call expression in the same block
is a candidate.

## Why now

Used `--ncc-lto-guard` on n00b-regex with LTO+O3 to defeat the regex parser
crashes documented in resharp-c's `~/dd/re-gc-tweak.md`. The struct-field
extension reduced the crash domain substantially (parser-stage SIGSEGVs
disappeared); the next bottleneck observed is in conduit/CV-wait code
which may or may not be a separate concurrency bug.

Volunteers: would gladly take a PR.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.