fix(go): Context copies byte-slice parts - #1022
Conversation
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The ownership fix is focused, consistently applied, and adequately tested.
Review effort: Balanced
Findings: None
What changed in this PR
Ensures Go encryption contexts retain immutable byte-slice parts.
Changes:
- Adds shared byte-slice ownership logic.
- Applies it to contexts and context extensions.
- Tests mutation resistance for root and extended parts.
| File | Description |
|---|---|
languages/golang/stackencrypt/context.go |
Copies byte-slice context parts. |
languages/golang/stackencrypt/record.go |
Reuses the shared ownership helper. |
languages/golang/stackencrypt/unit_test.go |
Verifies contexts resist caller-buffer mutation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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. |
|
Rebased onto the updated |
43bdf47 to
f1cdbb8
Compare
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
The change is correct and small. I approve it.
ownPartcopies a[]bytepart. It passes all other part types through, and those types are values (string and integers).NewContextandWithnow use it, so aContextdoes not alias a buffer of the caller.ExtendContextuses the same helper, so the copy rule has one definition.NewContextandContext.Withare the only two places that setContext.node. Thus no other path can store an aliased slice.bytes.Clonekeeps a nil slice as a nil[]byte, andcheckPartruns before the copy. Thus the validation and the type the codec gets do not change.TestContextOwnsItsBytePartsoverwrites both source buffers after the context is built, and it checks each part. It covers the root path and theWithpath.
No changeset is necessary: the Go module does not have a release process yet, and no skills/* file documents it.
f1cdbb8 to
887529d
Compare
NewContext and Context.With stored a []byte part as the caller's slice, the same aliasing ExtendContext had one layer up: a buffer reused after the context was built changed what the context bound. One helper, ownPart, now copies a byte part for both the Context constructors and the option, so there is a single definition of what a part the SDK holds looks like. A test mutates the source buffers after NewContext and With and checks the context still holds the original bytes.
887529d to
ab8ca58
Compare
Summary
Stacked on #1019. That PR made
ExtendContextcopy a byte-slice part so an option held across calls cannot follow later changes to the caller's buffer. TheContexttype itself had the same gap one layer down:NewContextandContext.Withstored a[]bytepart as the caller's slice. AContextis usually built and used in one call, so this rarely bites, but a context kept and reused would silently bind different bytes if the buffer behind it was rewritten.Both now go through one helper, so there is a single definition of how the SDK holds a part.
Changes
context.go:ownPartcopies a byte-slice part and passes every other part through.NewContextandWithuse it, and theContextdoc says the type owns its parts.record.go:ExtendContextuses the same helper instead of its own copy loop.TestContextOwnsItsBytePartsbuilds a context from two byte buffers, overwrites both, and checks the context still holds the original bytes.Verification
ownParttemporarily made an identity, bothTestContextOwnsItsBytePartsandTestExtendContextOwnsItsBytePartsfail, naming the mutated bytes; with it restored, both pass.go vet, gofmt, golangci-lint (0 issues) and everystackencrypttest package pass with the built guest.No changeset: the Go module has no release process yet.
Related