Fix incompleteness with nil checking change - #1077
Conversation
Using an L-value in Go panics if and only if the pointer dereferenced at the root of the L-value chain is nil; bound violations are covered by separate checks. Previously, the encoding asserted non-nilness of the address of the entire L-value, which the verifier often cannot prove for compound locations (e.g. elements of a resliced slice), because the obligation requires matching quantified permissions modulo the offset arithmetic of the slice and array encodings. Checking the root pointer instead removes these spurious obligations for variable-rooted L-values and turns the remaining obligations into simple pointer comparisons. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uq8bfbn2m7dJ98r5WuSjMT
Specifications and intrinsically ghost statements (assert, inhale, exhale, fold, unfold) are never executed and thus cannot panic. There, an L-value rooted in a nil pointer denotes an unconstrained value, and the permission system already prevents deriving any facts about actual memory from it: a nil-receiver predicate instance can never be folded, and permissions to nil-rooted locations can never be exhaled by a caller. Emitting the checks in specifications also wraps receiver terms in the typedAssert function inside predicate bodies and pure function definitions, which perturbs triggers and snapshot structure and destabilizes proofs that depend on them. The suppression is threaded through an immutable flag on the encoding context: specification entry points (assertion, invariant, precondition, postcondition) and pure function bodies encode with a derived context whose emitNilChecks flag is false. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uq8bfbn2m7dJ98r5WuSjMT
…checks Checking that usages of L-values do not dereference nil introduces proof obligations that VerifiedSCION cannot discharge where the relevant permissions are wrapped in predicates: a predicate instance implies nothing about the non-nilness of its receiver, so a method that calls a method on a field of its receiver (the mutex idiom, `d.mtx.Lock()`) cannot prove that the receiver is non-nil while `d.Mem()` stays folded. This patch adds the 13 spec annotations that discharge them, so that the companion changes required in VerifiedSCION are reviewable alongside this PR. It applies to VerifiedSCION at 41b5316. Status: the pkg/slayers conjunct is verified (that package goes from 1 error to 0). The 12 router annotations parse, type-check and encode cleanly, and a partial router run with them reported no errors where the unannotated run reported all 12 within minutes; the router package could not be verified to completion in the sandbox used, so they still require a CI-sized run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uq8bfbn2m7dJ98r5WuSjMT
…message The documentation claimed that using an L-value panics if and only if the pointer at the root of its chain is nil. That is not true: a chain such as l.Next.Next.Next dereferences a pointer at every link, and any of them being nil causes a panic. Checking only the outermost dereference is nevertheless sufficient. The pointers dereferenced before it are read out of memory to form the chain, and reading them requires permission to the corresponding fields, which entails that they are non-nil; the outermost dereference is the only one without such a witness, as the memory it refers to is not necessarily read. The description now gives this argument. The reason of the error named the entire L-value instead of the pointer that might be nil, because the position of an implicit dereference spans the entire selector. The annotation is now attached to the information of the pointer, so that `&l.Next.Next.Val` reports that `l.Next.Next` might be nil. Adds tests for long chains of L-values that may dereference nil, covering both the explicitly checked dereference and the ones that are rejected because the permission required to read them is missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uq8bfbn2m7dJ98r5WuSjMT
Only the outermost dereference of an L-value chain gives rise to a nil-ness proof obligation; the dereferences before it are rejected by the permission required to read the pointer that follows them. The tests now document which of the two mechanisms rejects each case, so that the permission errors are not mistaken for missing nil checks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uq8bfbn2m7dJ98r5WuSjMT
Emits nil checks in the bodies of pure functions again. A pure function that is not ghost is executable, so a dereference in its body may panic and has to be checked. Suppressing the checks there was not necessary: the bodies of ghost pure functions unfold the predicates that carry the permissions entailing that the dereferenced pointers are non-nil. Shortens the description of `checkNotNil` and states which dereference is checked with an example, as the phrase "last dereference" is ambiguous: evaluating `l.next.val` dereferences `l` first and `l.next` last, and it is the latter that is checked. Renames `nilCheckSource` to `lastDeref` and `rootPointerNotNil` to `dereferencedPointerNotNil`, which no longer describes `d` through an equation, and makes `lastDeref` enumerate the cases of a location instead of using a catch-all. Adds tests for the bodies of pure functions that are not ghost, and for taking the address of a field of a pointer that is not known to be non-nil. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uq8bfbn2m7dJ98r5WuSjMT
Renames `lastDeref` to `outermostDeref` and describes it as the dereference that is not nested inside another dereference of the location, instead of the one that is applied last when the location is evaluated. The latter is easily read as its opposite. Adds tests for index expressions whose base is a slice expression, which is not a location and therefore has no outermost dereference of its own. Slicing an array pointer dereferences it, and that dereference is checked when the slice expression is encoded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uq8bfbn2m7dJ98r5WuSjMT
3737d68 to
504fe4e
Compare
Why is that? Is this something that Gobra simply enforces (even though we technically won't have to)? |
| * Usages of L-values are: (1) taking a reference, (2) taking a slice, (3) converting to R-value | ||
| * | ||
| * SafeRef[loc: T@] => assert [&loc != nil: *T°]; Ref[loc] | ||
| * SafeRef[loc: T@] => assert [p != nil]; Ref[loc] where *p is the root of loc (if any, see checkNotNil) |
There was a problem hiding this comment.
This was a stale comment; It's the outermost dereference, i.e. the last one evaluated (for l.next.val, the dereference of l.next) — what outermostDeref returns. Reworded.
| * Encodes the non-nilness of the address of an L-value. Used as the footprint of zero-sized types | ||
| * (which have no permission footprint), not as a runtime-panic check. |
There was a problem hiding this comment.
What does it mean to use this function as the footprint of zeros-zed types and not as a runtime-panic check?
There was a problem hiding this comment.
A location of a zero-sized type (struct{}, [0]int, …) occupies no memory, so no permission can constitute its footprint — acc(x) would degenerate to true and an allocated *struct{} would be indistinguishable from a nil one. Gobra therefore defines the footprint of such a location to be the non-nilness of its address. So this is not an obligation imposed on a usage of the L-value (that is the other checkNotNil, which asserts before a use); it is inhaled and exhaled where the permissions of a non-zero-sized type would be: inhaled by initialization/allocate and the New encoding, exhaled and re-inhaled by assignment, and exhaled wherever acc(loc) is exhaled via ctx.footprint. The two functions used to be one, because the panic check was previously stated over &loc; now that it checks the dereferenced pointer instead, they share nothing but the name, so this one is renamed to addressNotNil and documented as above.
It is a convention; we could definitely consider alternative designs. |
Indexing and slicing a pointer to an array implicitly dereference the
pointer ('p[i]' is shorthand for '(*p)[i]'), but the desugarer kept the
pointer as the direct base of the indexed or sliced expression. The
structural search for the outermost dereference of an L-value therefore
found no dereference and usages such as '&p[0]' or 'p[1:3]' generated
no nil-ness proof obligation at all, unlike the analogous field
accesses. The desugarer now makes the implicit dereference explicit
such that all further processing can uniformly treat the base as an
array; this also covers defined types whose underlying type is a
pointer to an array.
The added test cases document the different behavior of []byte and
*[N]byte, including that permission to the array does not (yet) entail
non-nilness of the pointer, in contrast to structs, since the
shared-array footprint consists of quantified permissions over embedded
locations rather than field permissions on the dereferenced pointer.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
In Go, indexing panics if the index is not within the length (only slicing is bounded by the capacity); this includes taking the address of an element. Previously, no bounds obligation was generated on this path: reads and writes are guarded by their permission footprints, which range over the in-bounds indices only, but taking the address of an element is encoded with total domain functions and, thus, verified unconditionally, e.g., for '&s[0]' with a possibly empty slice s. The obligation is emitted by safeReference for every indexed access of an array or a slice on the L-value's access path and, like the nil checks, applies only to actual code. In particular, the internal capacity-ranged footprint of 'make' and quantified permissions in specifications are unaffected. The conditions are emitted innermost-first, each guarded by the inner conditions, such that the well-definedness and the error of an outer condition can rely on the inner indices being in bounds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Triggers are patterns that are never evaluated; wrapping them in the panic-absence checks' function applications would render them invalid as triggers. The exemption also applies to quantifiers in expression positions within actual code, e.g., in pure function bodies or ghost assignments. Furthermore, in Go, 'len' of an operand of array type is a constant and the operand is not evaluated; hence, neither the bounds of indices in the operand nor the non-nilness of a dereferenced pointer are checked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The flag now gates both the nil-dereference checks and the index-bounds checks, i.e., the checks for the absence of runtime panics in general. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ases for nested indexed accesses The implication guarding an outer bounds condition was created with the indexed expression's plain meta information, such that the error lost the annotation identifying it as an index-bounds failure. The added test cases document the behavior of nested indexed accesses: computing the address of a nested element reads the inner slice header, which requires permission and, thus, an in-bounds inner index. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… type's length In Go, the length and the capacity of a pointer to an array are the length constant of the array type; this holds even for a nil pointer. Previously, both were encoded by dereferencing the pointer, which made them unprovable for pointers that are not known to be non-nil. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Describe the checked pointer in safeReference as the outermost dereference instead of the undefined "root", and rename the footprint of zero-sized locations to addressNotNil, so that it is no longer an overload of the runtime-panic check it no longer shares an encoding with. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0131AHEJowteuvuWW9XCj1PQ
Gloss the outermost dereference where it is used, so that the checked pointer is identified without following the helper, and state what addressNotNil is: the footprint of a location that owns no permission, rather than an obligation on a usage of that location. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0131AHEJowteuvuWW9XCj1PQ
PR #531 makes Gobra report a possible panic whenever a program dereferences a pointer that may be nil. The checks it added were too strict, and rejected correct programs (see the comments in that PR for examples). This PR changes what is checked and where.
Correct programs were rejected
We own every element of raw, and nothing here is even a pointer that could be nil. The old check asked whether the address of the element is non-nil. Gobra computes that address by combining the offset of the sub-slice with the index, and the verifier could not relate the result to the permissions we hold, which are written in terms of raw[i].
The new check asks a simpler question: which pointer is dereferenced here, and can it be nil? In the example, none is — raw is a variable, and a slice already carries its own array — so no proof obligation is generated at all. When a pointer is dereferenced, as in p.f, the obligation is just p != nil, which is normally immediate from the surrounding specification.
Checks were reported in specifications
A predicate definition is not executed, so it cannot panic — and a predicate about a nil receiver can never be folded in the first place. The check nevertheless fired here, before the definition had mentioned any permission. Putting checks into specifications also changed the shape of the formulas the prover sees, which made unrelated proofs fail elsewhere. Checks are now generated only in code that actually runs; bodies of pure functions keep theirs, since a pure function that is not ghost is real code. In the future, we could disable the checks in the bodies of pure ghost functions, but we don't do it atm.