Open johejo opened 3 years ago
I'm seeing the same thing - oddly enough, it doesn't panic if pass something like []byte("[]wazza wazza woo")
, but does panic if I pass slightly invalid json (missing a closing quote on level1.thing1):
{
"level1": {
"thing1: {
"somevalue": []
},
"thing2": {
"usefulthing": "bleh"
}
},
"level2": {
"widget1": {
"meh": {}
}
}
}
Its panicing in parser.validateMapValue on this line: valueColumn := value.GetToken().Position.Column
It seems that parser.parseMapValue (called in parser.parseMappingValue) can return nil for value. Should a check be added to parser.parseMapValue to return an error if value is nil? It looks like parser.parseToken can return nil values for several conditions, but looking at other areas of the codebase (such as ast.Integer as is one of the stack traces above), the code assumes the value is not nil. Should parser.parseToken simply return an error if value is nil?
@goccy gentle ping
func TestBadInput(t *testing.T) {
for _, tt := range []struct{
Input string
} {
{Input: "0::"},
{Input: "{0"},
{Input: "*-0"},
{Input: ">\n>"},
{Input: "&{0"},
{Input: "0_"},
} {
t.Run(tt.Input, func(t *testing.T) {
// Should not panic.
var i interface{}
_ = yaml.Unmarshal([]byte(tt.Input), &i)
})
}
}
Does not panic with such patch:
diff --git a/ast/ast.go b/ast/ast.go
index 83b1ee6..5d528df 100644
--- a/ast/ast.go
+++ b/ast/ast.go
@@ -328,7 +329,7 @@ func Integer(tk *token.Token) Node {
}
negativePrefix = "-"
} else {
- if value[1] == 'o' {
+ if len(value) > 1 && value[1] == 'o' {
skipCharacterNum++
}
}
diff --git a/parser/parser.go b/parser/parser.go
index fcbd3bd..5ebb822 100644
--- a/parser/parser.go
+++ b/parser/parser.go
@@ -185,6 +186,9 @@ func (p *parser) createMapValueNode(ctx *context, key ast.Node, colonToken *toke
}
func (p *parser) validateMapValue(ctx *context, key, value ast.Node) error {
+ if value == nil {
+ return nil
+ }
keyColumn := key.GetToken().Position.Column
valueColumn := value.GetToken().Position.Column
if keyColumn != valueColumn {
@@ -449,6 +453,9 @@ func (p *parser) parseLiteral(ctx *context) (ast.Node, error) {
ctx.progress(1) // skip literal/folded token
tk := ctx.currentToken()
+ if tk == nil {
+ return nil, errors.ErrSyntax("expected token, but got none", tk)
+ }
var comment *ast.CommentGroupNode
if tk.Type == token.CommentType {
comment = p.parseCommentOnly(ctx)
But I'm not sure that it is correct because I don't know yaml spec.
This issue concerns me as potential DoS attack vector, I can submit a PR if my patch is ok (but I still think that you know better way to deal with such cases).
I can reproduce a similar panic with input data [invalid:,
: https://go.dev/play/p/mZ3btWag26C. This makes the library unusable for us.
Found in go-fuzz https://github.com/dvyukov/go-fuzz
Bad inputs have panic. Some may have the same cause.
Output