Repository navigation
feat(vcffi): write a nil fixed-width pointer as a typed null, and read typed nulls - #499
Conversation
🧬 Mutation testing (cargo-mutants,
|
✅ No CRAP threshold violations1329 function(s) analyzed · threshold 30 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes (1 of 4 review job(s) failed)
Before merging, fix the endless loop in fixedTypeOf for pointer types that point at themselves, such as type P *P. Also add the three small tests that protect the rule "one encoding per absence": the vcvalue.Uint128 marker row, the UUID parameter vector, and the Undefined and Decimal envelope vectors. The other comments can wait for a follow-up.
Other findings not posted as comments
bindings/go/vcencrypt/typed_null_test.go:18: the guest length test covers 6 of 16 fixed-width types. Add int8, int16, int64, Int128, uint8, uint16, uint32, uint64, Uint128 and float32 tooptional. Optional.bindings/go/vcffi/encoder.go(vcvalue.Plainmarker): no test sends a nil fixed-width pointer insidevcvalue.Plainthrough the guest.Plain{V: (*int32)(nil)}writes a typed null inside a passthrough frame. Optional.bindings/go/vcffi/typed_null_test.go:121: the table tests uset.Fatalfwith not.Run, so the first failure hides the others. Optional.bindings/go/vcffi/encoder.go:600: no test covers a non-nil outer pointer around a nil fixed-width inner pointer, for exampleMarshal(&missing)withvar missing *int32. The expected result is1504000000. Optional.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 7 found, 5 posted |
| claude | claude-opus-5-5 | golang | 3 found, 2 posted |
| codex | gpt-5.6-terra | test-gap | 1 found, 0 posted |
| codex | gpt-5.6-terra | golang | failed |
Synthesis: claude-sonnet-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 0 posted finding(s) were raised by two or more models.
Plain language: claude-sonnet-5-5 read every comment as a new reader would. 2 comment(s) had a problem that stopped the reader acting; it rewrote 2. It also rewrote the review body.
Stack: position 2 of 2 (#497, *️⃣ #499). *️⃣ marks this pull request.
Context loaded: the description, 1 linked issue(s) and 23 discussion entries.
…m alike Nine normalized vectors (a typed null of each kind reads as null and is written back as the plain null, as a frame writer writes it) and 22 malformed ones (padding that is not zero or the wrong length, a tag or an envelope with no typed null). Rust, Go and Node read the corpus directly. Refs #493
…d typed nulls, so Go hides absent values too A nil *int32, *float64, *time.Time, *vcvalue.Date, *vcvalue.Decimal, *vcvalue.UUID (a named type over one included) now encodes as a typed null, padded to the length of a value of its type, as Rust writes an Option<T>'s None. vcvalue.TypedNull and Encoder.TypedNull cover a caller whose Go type cannot say what is missing. A nil with no fixed-width type (an interface, a *bool, a *string, an Encryptable) stays the plain null. Unmarshal reads a typed null as strictly as the Rust codec and decodes it as nil, as Rust's frame writer writes it back as the plain null. The committed guest is rebuilt from this tree so the differential fuzz targets compare Go with the Rust that reads typed nulls, and their walkers learn the projection. Refs #493
…their padding, sharper tests and docs - Drive interceptMarkers and the numeric kinds of encodeReflect from the same tables fixedTypeOf reads, so a new marker type cannot be written one way and padded another; a test checks every fixed-width entry against a sample value, and fails for a marker added without one. fixedTypeOf now runs only for a nil pointer and is cached per type. - Share the UUID envelope header between Encoder.UUID and its typed null, and check Encoder.TypedNull's type with FixedType.Valid. - Make the corpus's uuid-with-length vector the full 24 bytes, so it tests a length prefix rather than truncation. - Check the *string half of the plain-null test. - Say in the README that only named int and float types are padded, that a value re-encrypted from any or vcvalue.Object seals its nils as the plain null, and that a binding must update its guest with this module. - Hoist the differential walker's width tables to one shared table. Refs #493
…l-pointer type walk, and pin the typed-null edges the review found A pointer type that points at itself (type P *P) never reaches a non-pointer, so fixedTypeOf looped forever on a nil value of it and Marshal never returned. The walk is now bounded by the same depth budget that bounds a value cycle, and past it the type keeps the plain null. An Encryptable that always writes one value of one fixed-width type had no way to say so, so its nil pointer leaked absence by length where a plain *int64 did not. FixedWidth is the Go side of Rust's Encrypt::FIXED: a nil pointer to a type that implements it is a typed null of the type it declares, and an invalid declaration fails the encode rather than write the plain null in its place. The shared corpus gains three refusals every binding now checks: a uuid typed null with a nonzero parameter, and a typed null in the envelope under the undefined or decimal id, which Go accepted only because no vector reached those arms. The nil-pointer table covers every fixed-width type, a non-nil pointer to a nil one, a pointer to a pointer to an Encryptable with either receiver, and reads the per-type cache; the header test fails for a sample whose marker lost its fixed type; and the guest tests seal an optional field of all sixteen types and a nil fixed-width pointer inside a passthrough.
1ea3b59 to
8735de7
Compare
Closes #493
Stacked on #497 (base:
feat/aead-typed-null). Review that first; this PR's diff is only the Go side and the shared corpus.Why
#497 makes Rust seal a fixed-width
Noneas a typed null, the same length as a value, so ciphertext length doesn't show which optional values are absent. The Go codec (vcffi) is what the Go SDK (cipherstash/stack#1298) is meant to share. Until this PR it could neither write a typed null nor read one, so Go-encrypted optional fields still leaked absence by length.What changes for a Go caller
Option<T>. That covers*int8…*int64,*int, the unsigned widths,*float32,*float64,*time.Time,*vcvalue.Date,*vcvalue.Timestamp,*vcvalue.Decimal,*vcvalue.UUID,*vcvalue.Int128,*vcvalue.Uint128, a named int or float type (type Age int32), and through any number of pointers.*bool(already 1 byte),*stringand*[]byte(variable width),*[N]byte, a pointer to a struct (a named type over avcvalueortimetype included, since reflection writes it field by field), and a pointer to anEncryptable, which chooses its own encoding.map[string]any), there isvcvalue.TypedNull{Type: vcvalue.TypeDate}. AnEncryptablecan callEncoder.TypedNull(vcvalue.TypeInt32).vcvalue.FixedTypenames the 16 fixed-width types.EncryptablewhoseEncryptValuealways writes one value of one fixed-width type can implementvcffi.FixedWidth(FixedType() vcvalue.FixedType), the Go side of Rust'sEncrypt::FIXED. A nil pointer to it is then a typed null of that type.FixedTypeis called once per type on a pointer to a zero value, and an invalid declaration fails the encode withErrInvalidFixedTyperather than write the plain null.Unmarshalreads a typed null and decodes it asnil, like every null. Its validation is as strict as Rust's: header, then exactly the zero padding, so every absence has one encoding. Rust's frame writer writes a typed null back as the plain null (feat(aead)!: seal a fixed-width None as a typed null as long as a Some #497), so Go does too.Wire format
The same as #497 and pinned against the same bytes. For example,
*int32nil gives15 04 00 00 00,*vcvalue.Datenil gives15 12 00 00 00, and*vcvalue.UUIDnil gives15 F3 05 00 0B+ 15 zeros.TestTypedNullKnownAnswerspins all 16 types and checks each against a value's length.Shared corpus
testdata/value-conformance.jsongains:00.0xF4); an envelope over a shorthand type, id zero, the undefined id, the decimal id, timestamp nanos, a uuid over an int, a uuid with a length prefix, a uuid with a nonzero parameter, a timestamp over a float, an unknown id over bytes or bool; and a typed null inside an envelope.Rust (
vitaminc-aead's leaf test,vitaminc-aead-frame's conformance test), Go (vcffi) and Node (value-conformance.cjs, through the napi addon) all read it directly, and all pass.Guest rebuild
bindings/go/vcencrypt/wasm/vitaminc_guest.wasmis rebuilt from this tree withcargo build --release --target wasm32-wasip1, asvcencrypt/README.mdsays. The differential fuzz targets compare Go's decoder with the committed guest, which now accepts typed nulls in the frames it encrypts. Their walkers (rewritein vcencrypt,canonicalFormin vcffi) learn the projection: a typed null re-encodes as00on both sides.Trade-offs to review
*int32used to seal as the 1-byte null and now seals padded. That is the point, and it matches Rust. A reader built before feat(aead)!: seal a fixed-width None as a typed null as long as a Some #497 refuses the new leaf.anyloses the type. Since feat(aead)!: seal a fixed-width None as a typed null as long as a Some #497, Rust's frame writer writes typed nulls as the plain null, so a decrypted frame never carries one. A value re-encrypted fromanyorvcvalue.Objecttherefore seals its nils as the plain null. Hold optional fields in a typed struct (Born *vcvalue.Date) before encrypting again. The README says so.vcffi; a guest that predates feat(aead)!: seal a fixed-width None as a typed null as long as a Some #497 refuses0x15. The README says so.[N]bytestays the plain null. A plain bytes typed null has no length in its header to say where its padding ends in a frame (the same call as feat(aead)!: seal a fixed-width None as a typed null as long as a Some #497).Review
A high-effort review posted 10 inline findings, with no correctness bugs among them. Eight are addressed in 1283e1b:
interceptMarkers,encodeReflect's numeric arms andfixedTypeOfnow share one table, checked against a sample of every fixed-width entry.Valid()is used.*stringcheck is added.fixedTypeOfis cached and runs only for nil pointers.Two are answered without code changes, as above: decrypt then re-encrypt through
any, and the opt-out. One more is declined with reasons in its thread: a compile-time-closedFixedTypeisn't expressible in Go, and an invalid one fails closed withErrInvalidFixedType.A second, automated review posted 7 findings and listed 4 more as optional. All 11 are addressed in 8735de7, with a reply on each posted thread:
Marshalhung on a nil value of a pointer type that points at itself (type P *P):fixedTypeOf's walk is now bounded bymaxDepth, and such a type keeps the plain null.param != 0and id checks have tests in every binding.vcffi.FixedWidth, so anEncryptablethat always writes one fixed-width value can have its nil pointer padded.*vcvalue.Uint128was missing), a non-nil pointer to a nil*int32,**Encryptablewith either receiver, every case encoded twice so the cache is read, subtests instead of oneFatalf, a sample whose marker lost its fixed type fails the header test, the guest length test covers all 16 types, and a nil fixed-width pointer insidevcvalue.Plaingoes through the guest and comes back as the plain null.Testing
go vetandgo testpass forvcvalue,vcffiandvcencrypt.go vetpasses forGOOS=linux GOARCH=386, but the 386 tests were not run locally: they need a linux runner, which CI has.vcencrypt/typed_null_test.go, through the real guest: a struct with a nil field of each of the 16 fixed-width types seals every leaf to the same length as a present one, and decrypts as nil. A nil*boolmatches a present bool, a nil*stringan empty string, and a nil*int32insidevcvalue.Plainis the plain null in the ciphertext and after a decrypt.vcffi/typed_null_test.go: known answers, the nil-pointer table (every fixed-width type, pointers to pointers,Encryptables with either receiver, a self-pointing type, each encoded twice),FixedWidth, struct fields, the explicit channel, and strict decode edges.FuzzUnmarshalran for 20s andFuzzDifferentialValuefor 30s against the rebuilt guest, with no failures.cargo fmt --check, clippy (wasm32-wasip1 and native) with-D warnings, andcargo testall pass. Theuserrecordexample runs.vitaminc-aeadtests (all features) and thevitaminc-aead-frameconformance tests pass. Node conformance reports 34 values and 70 refusals.