Skip to content

Fix panic unmarshaling a child node that shares a parent property name - #14

Open
ChrisJr404 wants to merge 1 commit into
sblinch:mainfrom
ChrisJr404:fix-unmarshal-interface-panic
Open

ChrisJr404 wants to merge 1 commit into
sblinch:mainfrom
ChrisJr404:fix-unmarshal-interface-panic

Conversation

@ChrisJr404

Copy link
Copy Markdown

Summary

Unmarshal panics on certain malformed-but-parseable documents when a node
carries both a property and a child node that share the same name.

Minimal reproducer:

var m map[string]interface{}
err := kdl.Unmarshal([]byte(`A b="" {b 0x0`), &m)

This panics with:

panic: reflect: reflect.Value.Set using unaddressable value

Other value shapes for the child argument trigger it too (1.5, true, a
hex literal, multiple arguments), and it also reproduces with a well-formed
closing brace once the name collision is present.

Cause

In the reflect.Interface branch of unmarshalNodeToValue, the target value
is derived with:

v := *destVal
if v.IsValid() && v.Elem().IsValid() {
    v = v.Elem()
}

When the map key already holds a concrete value (because the parent property
of the same name was unmarshaled first), v.Elem() returns an unaddressable
value. The single-argument and multi-argument sub-branches then call v.Set(...)
unconditionally, which panics. The third sub-branch (the map case) already
guards this with v.CanSet() and falls back to writing through *dest.

Fix

Apply the same CanSet() guard to the single-argument and multi-argument
branches, falling back to assigning through the dest pointer so
withCreatedAndIndirected propagates the result — matching the existing map
branch. No change to the happy path.

Added TestBug10 covering the crashing inputs; it panics before the change and
passes after.

…y name

When a node has both a property and a child node that share the same name,
the map entry for that key already holds a concrete value (from the property)
by the time the child node is unmarshaled into an interface{}. Deriving the
target with reflect.Value.Elem() yields an unaddressable value, so the
single-argument and multi-argument interface branches panicked with
"reflect.Value.Set using unaddressable value".

Guard those Set calls with CanSet and fall back to assigning through the
dest pointer, matching the existing map branch. Adds a regression test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant