Skip to content

Commit

Permalink
Merge branch 'go-yaml:v3' into error-position-plus-failures
Browse files Browse the repository at this point in the history
  • Loading branch information
@andig
andig committed Apr 20, 2024
2 parents df93c53 + f6f7691 commit 9dda667
Show file tree
Hide file tree
Showing 4 changed files with 184 additions and 44 deletions.
16 changes: 0 additions & 16 deletions .github/workflows/semgrep.yaml
View file

This file was deleted.

78 changes: 64 additions & 14 deletions decode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,10 @@ func (p *parser) peek() yaml_event_type_t {
if p.event.typ != yaml_NO_EVENT {
return p.event.typ
}
if !yaml_parser_parse(&p.parser, &p.event) {
// It's curious choice from the underlying API to generally return a
// positive result on success, but on this case return true in an error
// scenario. This was the source of bugs in the past (issue #666).
if !yaml_parser_parse(&p.parser, &p.event) || p.parser.error != yaml_NO_ERROR {
p.fail()
}
return p.event.typ
Expand Down Expand Up @@ -315,6 +318,8 @@ type decoder struct {
decodeCount int
aliasCount int
aliasDepth int

mergedFields map[interface{}]bool
}

var (
Expand Down Expand Up @@ -811,18 +816,30 @@ func (d *decoder) mapping(n *Node, out reflect.Value) (good bool) {
}
}

mergedFields := d.mergedFields
d.mergedFields = nil

var mergeNode *Node

mapIsNew := false
if out.IsNil() {
out.Set(reflect.MakeMap(outt))
mapIsNew = true
}
for i := 0; i < l; i += 2 {
if isMerge(n.Content[i]) {
d.merge(n.Content[i+1], out)
mergeNode = n.Content[i+1]
continue
}
k := reflect.New(kt).Elem()
if d.unmarshal(n.Content[i], k) {
if mergedFields != nil {
ki := k.Interface()
if mergedFields[ki] {
continue
}
mergedFields[ki] = true
}
kkind := k.Kind()
if kkind == reflect.Interface {
kkind = k.Elem().Kind()
Expand All @@ -836,6 +853,12 @@ func (d *decoder) mapping(n *Node, out reflect.Value) (good bool) {
}
}
}

d.mergedFields = mergedFields
if mergeNode != nil {
d.merge(n, mergeNode, out)
}

d.stringMapType = stringMapType
d.generalMapType = generalMapType
return true
Expand All @@ -847,7 +870,8 @@ func isStringMap(n *Node) bool {
}
l := len(n.Content)
for i := 0; i < l; i += 2 {
if n.Content[i].ShortTag() != strTag {
shortTag := n.Content[i].ShortTag()
if shortTag != strTag && shortTag != mergeTag {
return false
}
}
Expand All @@ -864,7 +888,6 @@ func (d *decoder) mappingStruct(n *Node, out reflect.Value) (good bool) {
var elemType reflect.Type
if sinfo.InlineMap != -1 {
inlineMap = out.Field(sinfo.InlineMap)
inlineMap.Set(reflect.New(inlineMap.Type()).Elem())
elemType = inlineMap.Type().Elem()
}

Expand All @@ -873,6 +896,9 @@ func (d *decoder) mappingStruct(n *Node, out reflect.Value) (good bool) {
d.prepare(n, field)
}

mergedFields := d.mergedFields
d.mergedFields = nil
var mergeNode *Node
var doneFields []bool
if d.uniqueKeys {
doneFields = make([]bool, len(sinfo.FieldsList))
Expand All @@ -882,13 +908,20 @@ func (d *decoder) mappingStruct(n *Node, out reflect.Value) (good bool) {
for i := 0; i < l; i += 2 {
ni := n.Content[i]
if isMerge(ni) {
d.merge(n.Content[i+1], out)
mergeNode = n.Content[i+1]
continue
}
if !d.unmarshal(ni, name) {
continue
}
if info, ok := sinfo.FieldsMap[name.String()]; ok {
sname := name.String()
if mergedFields != nil {
if mergedFields[sname] {
continue
}
mergedFields[sname] = true
}
if info, ok := sinfo.FieldsMap[sname]; ok {
if d.uniqueKeys {
if doneFields[info.Id] {
d.terrors = append(d.terrors, UnmarshalError{
Expand Down Expand Up @@ -922,26 +955,41 @@ func (d *decoder) mappingStruct(n *Node, out reflect.Value) (good bool) {
})
}
}

d.mergedFields = mergedFields
if mergeNode != nil {
d.merge(n, mergeNode, out)
}
return true
}

func failWantMap() {
failf("map merge requires map or sequence of maps as the value")
}

func (d *decoder) merge(n *Node, out reflect.Value) {
switch n.Kind {
func (d *decoder) merge(parent *Node, merge *Node, out reflect.Value) {
mergedFields := d.mergedFields
if mergedFields == nil {
d.mergedFields = make(map[interface{}]bool)
for i := 0; i < len(parent.Content); i += 2 {
k := reflect.New(ifaceType).Elem()
if d.unmarshal(parent.Content[i], k) {
d.mergedFields[k.Interface()] = true
}
}
}

switch merge.Kind {
case MappingNode:
d.unmarshal(n, out)
d.unmarshal(merge, out)
case AliasNode:
if n.Alias != nil && n.Alias.Kind != MappingNode {
if merge.Alias != nil && merge.Alias.Kind != MappingNode {
failWantMap()
}
d.unmarshal(n, out)
d.unmarshal(merge, out)
case SequenceNode:
// Step backwards as earlier nodes take precedence.
for i := len(n.Content) - 1; i >= 0; i-- {
ni := n.Content[i]
for i := 0; i < len(merge.Content); i++ {
ni := merge.Content[i]
if ni.Kind == AliasNode {
if ni.Alias != nil && ni.Alias.Kind != MappingNode {
failWantMap()
Expand All @@ -954,6 +1002,8 @@ func (d *decoder) merge(n *Node, out reflect.Value) {
default:
failWantMap()
}

d.mergedFields = mergedFields
}

func isMerge(n *Node) bool {
Expand Down
123 changes: 110 additions & 13 deletions decode_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,7 @@ var unmarshalTests = []struct {
M{"a": 123456e1},
}, {
"a: 123456E1\n",
M{"a": 123456E1},
M{"a": 123456e1},
},
// yaml-test-suite 3GZX: Spec Example 7.1. Alias Nodes
{
Expand Down Expand Up @@ -802,7 +802,6 @@ var unmarshalTests = []struct {
"c": []interface{}{"d", "e"},
},
},

}

type M map[string]interface{}
Expand Down Expand Up @@ -948,16 +947,18 @@ var unmarshalErrorTests = []struct {
{"%TAG !%79! tag:yaml.org,2002:\n---\nv: !%79!int '1'", "yaml: did not find expected whitespace"},
{"a:\n 1:\nb\n 2:", ".*could not find expected ':'"},
{"a: 1\nb: 2\nc 2\nd: 3\n", "^yaml: line 3: could not find expected ':'$"},
{"#\n-\n{", "yaml: line 3: could not find expected ':'"}, // Issue #665
{"0: [:!00 \xef", "yaml: incomplete UTF-8 octet sequence"}, // Issue #666
{
"a: &a [00,00,00,00,00,00,00,00,00]\n" +
"b: &b [*a,*a,*a,*a,*a,*a,*a,*a,*a]\n" +
"c: &c [*b,*b,*b,*b,*b,*b,*b,*b,*b]\n" +
"d: &d [*c,*c,*c,*c,*c,*c,*c,*c,*c]\n" +
"e: &e [*d,*d,*d,*d,*d,*d,*d,*d,*d]\n" +
"f: &f [*e,*e,*e,*e,*e,*e,*e,*e,*e]\n" +
"g: &g [*f,*f,*f,*f,*f,*f,*f,*f,*f]\n" +
"h: &h [*g,*g,*g,*g,*g,*g,*g,*g,*g]\n" +
"i: &i [*h,*h,*h,*h,*h,*h,*h,*h,*h]\n",
"b: &b [*a,*a,*a,*a,*a,*a,*a,*a,*a]\n" +
"c: &c [*b,*b,*b,*b,*b,*b,*b,*b,*b]\n" +
"d: &d [*c,*c,*c,*c,*c,*c,*c,*c,*c]\n" +
"e: &e [*d,*d,*d,*d,*d,*d,*d,*d,*d]\n" +
"f: &f [*e,*e,*e,*e,*e,*e,*e,*e,*e]\n" +
"g: &g [*f,*f,*f,*f,*f,*f,*f,*f,*f]\n" +
"h: &h [*g,*g,*g,*g,*g,*g,*g,*g,*g]\n" +
"i: &i [*h,*h,*h,*h,*h,*h,*h,*h,*h]\n",
"yaml: document contains excessive aliasing",
},
}
Expand Down Expand Up @@ -1403,7 +1404,7 @@ inlineSequenceMap:
`

func (s *S) TestMerge(c *C) {
var want = map[interface{}]interface{}{
var want = map[string]interface{}{
"x": 1,
"y": 2,
"r": 10,
Expand Down Expand Up @@ -1448,7 +1449,103 @@ func (s *S) TestMergeStruct(c *C) {
}
}

var unmarshalNullTests = []struct{ input string; pristine, expected func() interface{} }{{
var mergeTestsNested = `
mergeouter1: &mergeouter1
d: 40
e: 50
mergeouter2: &mergeouter2
e: 5
f: 6
g: 70
mergeinner1: &mergeinner1
<<: *mergeouter1
inner:
a: 1
b: 2
mergeinner2: &mergeinner2
<<: *mergeouter2
inner:
a: -1
b: -2
outer:
<<: [*mergeinner1, *mergeinner2]
f: 60
inner:
a: 10
`

func (s *S) TestMergeNestedStruct(c *C) {
// Issue #818: Merging used to just unmarshal twice on the target
// value, which worked for maps as these were replaced by the new map,
// but not on struct values as these are preserved. This resulted in
// the nested data from the merged map to be mixed up with the data
// from the map being merged into.
//
// This test also prevents two potential bugs from showing up:
//
// 1) A simple implementation might just zero out the nested value
// before unmarshaling the second time, but this would clobber previous
// data that is usually respected ({C: 30} below).
//
// 2) A simple implementation might attempt to handle the key skipping
// directly by iterating over the merging map without recursion, but
// there are more complex cases that require recursion.
//
// Quick summary of the fields:
//
// - A must come from outer and not overriden
// - B must not be set as its in the ignored merge
// - C should still be set as it's preset in the value
// - D should be set from the recursive merge
// - E should be set from the first recursive merge, ignored on the second
// - F should be set in the inlined map from outer, ignored later
// - G should be set in the inlined map from the second recursive merge
//

type Inner struct {
A, B, C int
}
type Outer struct {
D, E int
Inner Inner
Inline map[string]int `yaml:",inline"`
}
type Data struct {
Outer Outer
}

test := Data{Outer{0, 0, Inner{C: 30}, nil}}
want := Data{Outer{40, 50, Inner{A: 10, C: 30}, map[string]int{"f": 60, "g": 70}}}

err := yaml.Unmarshal([]byte(mergeTestsNested), &test)
c.Assert(err, IsNil)
c.Assert(test, DeepEquals, want)

// Repeat test with a map.

var testm map[string]interface{}
var wantm = map[string]interface {} {
"f": 60,
"inner": map[string]interface{}{
"a": 10,
},
"d": 40,
"e": 50,
"g": 70,
}
err = yaml.Unmarshal([]byte(mergeTestsNested), &testm)
c.Assert(err, IsNil)
c.Assert(testm["outer"], DeepEquals, wantm)
}

var unmarshalNullTests = []struct {
input string
pristine, expected func() interface{}
}{{
"null",
func() interface{} { var v interface{}; v = "v"; return &v },
func() interface{} { var v interface{}; v = nil; return &v },
Expand Down Expand Up @@ -1499,7 +1596,7 @@ func (s *S) TestUnmarshalNull(c *C) {
func (s *S) TestUnmarshalPreservesData(c *C) {
var v struct {
A, B int
C int `yaml:"-"`
C int `yaml:"-"`
}
v.A = 42
v.C = 88
Expand Down
11 changes: 10 additions & 1 deletion parserc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -687,6 +687,9 @@ func yaml_parser_parse_node(parser *yaml_parser_t, event *yaml_event_t, block, i
func yaml_parser_parse_block_sequence_entry(parser *yaml_parser_t, event *yaml_event_t, first bool) bool {
if first {
token := peek_token(parser)
if token == nil {
return false
}
parser.marks = append(parser.marks, token.start_mark)
skip_token(parser)
}
Expand Down Expand Up @@ -786,7 +789,7 @@ func yaml_parser_split_stem_comment(parser *yaml_parser_t, stem_len int) {
}

token := peek_token(parser)
if token.typ != yaml_BLOCK_SEQUENCE_START_TOKEN && token.typ != yaml_BLOCK_MAPPING_START_TOKEN {
if token == nil || token.typ != yaml_BLOCK_SEQUENCE_START_TOKEN && token.typ != yaml_BLOCK_MAPPING_START_TOKEN {
return
}

Expand All @@ -813,6 +816,9 @@ func yaml_parser_split_stem_comment(parser *yaml_parser_t, stem_len int) {
func yaml_parser_parse_block_mapping_key(parser *yaml_parser_t, event *yaml_event_t, first bool) bool {
if first {
token := peek_token(parser)
if token == nil {
return false
}
parser.marks = append(parser.marks, token.start_mark)
skip_token(parser)
}
Expand Down Expand Up @@ -922,6 +928,9 @@ func yaml_parser_parse_block_mapping_value(parser *yaml_parser_t, event *yaml_ev
func yaml_parser_parse_flow_sequence_entry(parser *yaml_parser_t, event *yaml_event_t, first bool) bool {
if first {
token := peek_token(parser)
if token == nil {
return false
}
parser.marks = append(parser.marks, token.start_mark)
skip_token(parser)
}
Expand Down

0 comments on commit 9dda667

Please sign in to comment.