fix: struct held in the interface word dereferences its pointer field twice - #602
Open
yES wants to merge 1 commit into
Open
fix: struct held in the interface word dereferences its pointer field twice#602yES wants to merge 1 commit into
yES wants to merge 1 commit into
Conversation
… twice A struct whose only field is a pointer is carried inside the interface word, so the encoder receives the pointer value where it would otherwise receive the address of the struct. compileStruct copies the parent's indirect property onto the child, so the child is compiled as non-indirect even though it is reached by its own address, and the surviving PtrCode wrapper makes the child head a StructPtrHead whose prologue follows the pointer a second time. Depending on which child fields are set the result is a dropped object (goccy#503) or an unrecoverable fatal error while the VM walks encoded bytes as a pointer. When the root struct is neither indirect nor reached by a pointer and the field is a single pointer to a struct, drop the redundant PtrCode, clear isNextOpPtrType and mark the child indirect. Types reachable from themselves are excluded: the recursive re-entry reuses the opcode block of the root, where the layout is the addressed one. Fixes goccy#503
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #503.
What is broken
A struct whose only field is a pointer is carried inside the interface word, so the encoder receives
the pointer value where it would otherwise receive the address of the struct. The compiled opcodes
assume the second layout and follow the field pointer once more than they should.
encoding/jsonOuter{}{}{}Outer{Inner: &Inner{}}{"inner":{}}{}Outer{Inner: &Inner{Flag: &flag}}{"inner":{"flag":true}}Outer{Inner: &Inner{Label: &label}}{"inner":{"label":"x"}}{}Outer{Inner: &Inner{Flag: &flag, Label: &label}}{"inner":{"flag":true,"label":"x"}}{"inner":{"flag":true}}The last row depends on what the process encoded earlier. Row 4 loses a populated object with no
error. Row 3 ends the process:
The fault address is ASCII fragments of the output buffer, so the VM is using encoded bytes as a
pointer.
fatal error: faultcannot be recovered, so a server hitting this goes down.Root cause
compileStructcopies the parent's indirect property onto the child struct:For a root struct in the interface word
indirectis false, so the child is compiled as non-indirecteven though it will be reached by its own address. The
PtrCodewrapper around it also survives,which turns the child head into
StructPtrHead*, and that prologue dereferences the slot a secondtime.
The compiled code for the failing case:
[000]stores the already-dereferenced child address into slot 1,[001]dereferences it again andleaves the
*boolfield value there, and[002]reads slot 1 + 8 expecting the struct base.The same mismatch drives the omit-empty decision.
OpStructHeadOmitEmptytestsptrToPtr(p) == 0tofind out whether the child pointer is nil, but
pis already the child address, so the test reads thechild's first field instead. That is the behaviour reported in #503: a child whose first field is zero
disappears.
Runtime trace of that test for the four values above:
pptrToPtr(p)Outer{}&Inner{}&Inner{Flag}&Inner{Label}The change
One file, no VM or generated code touched.
structCodetakes anisRootflag, set only bytypeToCode, which is the single entry point of acompilation. When the root struct is neither indirect nor reached by a pointer and the field is a
single pointer to a struct, the redundant
PtrCodeis unwrapped,isNextOpPtrTypeis cleared and thechild is marked indirect. The pointer has already been followed by the interface layout, so no opcode
should follow it again, and the omit-empty test that keys off
isNextOpPtrTypemust not run either.Types that are reachable from themselves are excluded: the recursive re-entry reuses the opcode block of the root, where the layout is the addressed one, so the transform cannot be correct there.
Both guards on the parent are necessary.
Marshal(&v)reaches the struct by a pointer, and a structstored in a map, a slice or a field is addressable, so in those cases the layout is the usual one and
nothing may change.
Testing
TestIssue503inencode_test.gocovers the shape above plus:interface{}fieldomitempty, a wider parent, two levels of nestingencoding/jsonforMarshal,MarshalIndentandEncoder.Encode, and run throughColorizeso all four VM variants are exercisedThe test fails on master, first with wrong output and then with the fault.
go test ./...andgo test -race ./...pass.Beyond the suite I ran a 98-shape corpus across
Marshal,MarshalIndent,Encoder.Encode,SetIndent,MarshalContext,MarshalNoEscape,Colorizeand colored indent, comparing every caseto
encoding/jsonon master and on this branch: 160 of the 784 combinations go from wrong or fatal tocorrect, and none change in any other way.
What this does not fix
struct{ R Root }) or embedded in another one-word struct. Master is equally broken there and this change leaves it alone rather than widening the crash.struct{ A [1]*Child }), which drops the element on master as well.omitemptypointer-to-struct field next to an embedded struct, when that struct is itself embedded (regression since v0.10.1) #581, Panic when marshaling nested struct with empty slice and omitempty tag #554). I checked Marshal: nil pointer dereference on anomitemptypointer-to-struct field next to an embedded struct, when that struct is itself embedded (regression since v0.10.1) #581 against this branch and it still panics, so this change and fix: embedded omitempty field dropped / nil-pointer panic in nested embedded structs (#576, #581, #554) #587 are independent.A one-word root holding a map, slice,
interface{}or a non-struct pointer is already correct onmaster: those specialised head opcodes gate their dereference on
IndirectFlags. The struct case isthe odd one out because the generic
OpStructHead*ops delegate that dereference to the child'sStructPtrHead*prologue, which has no such gate.