Skip to content

irverify: no invariant check that a BigVal in a document is a JSON-valid literal #282

Description

@OmarAlJarrah

Summary

ir.BigVal's doc comment states a contract: "A BigVal is always a JSON-valid
numeric literal". NewBigVal is what enforces it, and after #45 it enforces it
properly. But BigVal is a defined string type, so nothing stops a lowering
from writing one directly:

c.Min = &someBigVal          // fine, came from NewBigVal
*b.dst = ir.BigVal(raw)      // also compiles, and skips every check

irverify does not check the contract, so a document carrying such a value is
structurally sound as far as the verifier is concerned. It is only noticed
downstream, if at all — an emitter splicing the text into generated source, or
a consumer re-reading the IR as JSON.

Why irverify is the right home

This is the same hazard checkRawPayloads already covers for the other
verbatim-text carrier. ir.RawValue is copied into the output byte for byte,
so a payload that is not a JSON value breaks json.Marshal for the whole
document (invariant #7), and checkRawPayloads asserts it rather than trusting
the producer. BigVal is the same shape of risk with no equivalent check:
Constraints.Min/Max/MultipleOf/the exclusive bounds and Value.Num all
carry text that must read back as a JSON number.

The two channels stay distinct in the usual way: a spec that writes a bad
literal is an ir.Diagnostic from the compiler, and a lowering that puts one
into the document past NewBigVal is our bug, so a Violation is the right
report.

Suggested shape

A walking check over every BigVal-typed field reachable from the document,
asserting json.Valid([]byte(v)) and — since a JSON-valid string is not
necessarily what NewBigVal would have produced — that the value is its own
canonical form, i.e. NewBigVal(string(v)) returns it unchanged. The second
half is what would catch a leading zero or a stray + written directly.

Worth checking as part of the work whether the check actually reaches every
carrier: git grep -n 'BigVal' ir/ lists the fields it has to cover, and a
deliberately corrupted value planted at each is the way to confirm it does,
rather than assuming the walk gets there.

Context

Came up while fixing #45, which closed the constructor-side hole. Deliberately
left out of that change: it is an irverify feature rather than a NewBigVal
fix, and #45's scope was the parser.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions