From 70eecd52895b2bbfeddff6daded09f9e849d6374 Mon Sep 17 00:00:00 2001 From: James Shubin Date: Thu, 27 Feb 2020 18:52:02 -0500 Subject: [PATCH] lang: Prevent struct types with duplicate fields Struct types with duplicate fields are invalid types and weren't caught by the parser. This fixes the issue and adds some associated tests. It also checks and tests for duplicate struct value field names. As a technical side-note, this doesn't change the lang/types/ functions to remove panics-- the signatures are simplified to make their use simple, and we intentionally panic if they're used incorrectly. In this case, one was being used without having previously validated the input. Thanks to Patrick Meyer for finding this issue via fuzzing! --- lang/interpret_test.go | 35 ++++++++++++++++++- .../TestAstFunc2/struct-duplicate0.output | 1 + .../TestAstFunc2/struct-duplicate0/main.mcl | 1 + .../TestAstFunc2/struct-duplicate1.output | 1 + .../TestAstFunc2/struct-duplicate1/main.mcl | 1 + .../TestAstFunc2/struct-duplicate2.output | 1 + .../TestAstFunc2/struct-duplicate2/main.mcl | 1 + .../TestAstFunc2/struct-duplicate3.output | 1 + .../TestAstFunc2/struct-duplicate3/main.mcl | 2 ++ lang/parser.y | 31 ++++++++++++---- lang/structs.go | 8 +++++ 11 files changed, 75 insertions(+), 8 deletions(-) create mode 100644 lang/interpret_test/TestAstFunc2/struct-duplicate0.output create mode 100644 lang/interpret_test/TestAstFunc2/struct-duplicate0/main.mcl create mode 100644 lang/interpret_test/TestAstFunc2/struct-duplicate1.output create mode 100644 lang/interpret_test/TestAstFunc2/struct-duplicate1/main.mcl create mode 100644 lang/interpret_test/TestAstFunc2/struct-duplicate2.output create mode 100644 lang/interpret_test/TestAstFunc2/struct-duplicate2/main.mcl create mode 100644 lang/interpret_test/TestAstFunc2/struct-duplicate3.output create mode 100644 lang/interpret_test/TestAstFunc2/struct-duplicate3/main.mcl diff --git a/lang/interpret_test.go b/lang/interpret_test.go index a7fbb589..d229d31b 100644 --- a/lang/interpret_test.go +++ b/lang/interpret_test.go @@ -976,6 +976,8 @@ func TestAstFunc1(t *testing.T) { func TestAstFunc2(t *testing.T) { const magicError = "# err: " const magicError1 = "err1: " + const magicError9 = "err9: " // TODO: rename + // TODO: move them all down by one const magicError2 = "err2: " const magicError3 = "err3: " const magicError4 = "err4: " @@ -1010,6 +1012,8 @@ func TestAstFunc2(t *testing.T) { type errs struct { fail1 bool + fail9 bool // TODO: rename + // TODO: move them all down by one fail2 bool fail3 bool fail4 bool @@ -1068,6 +1072,8 @@ func TestAstFunc2(t *testing.T) { // if the graph file has a magic error string, it's a failure errStr := "" fail1 := false + fail9 := false // TODO: rename + // TODO: move them all down by one fail2 := false fail3 := false fail4 := false @@ -1082,6 +1088,12 @@ func TestAstFunc2(t *testing.T) { str = errStr fail1 = true } + if strings.HasPrefix(str, magicError9) { // TODO: rename + errStr = strings.TrimPrefix(str, magicError9) + str = errStr + fail9 = true + } + // TODO: move them all down by one if strings.HasPrefix(str, magicError2) { errStr = strings.TrimPrefix(str, magicError2) str = errStr @@ -1117,6 +1129,8 @@ func TestAstFunc2(t *testing.T) { expstr: str, errs: errs{ fail1: fail1, + fail9: fail9, // TODO: rename + // TODO: move them all down by one fail2: fail2, fail3: fail3, fail4: fail4, @@ -1156,6 +1170,8 @@ func TestAstFunc2(t *testing.T) { name, path, fail, expstr, errs := tc.name, tc.path, tc.fail, strings.Trim(tc.expstr, "\n"), tc.errs src := dir + path // location of the test fail1 := errs.fail1 + fail9 := errs.fail9 // TODO: rename + // TODO: move them all down by one fail2 := errs.fail2 fail3 := errs.fail3 fail4 := errs.fail4 @@ -1282,11 +1298,28 @@ func TestAstFunc2(t *testing.T) { }, } // some of this might happen *after* interpolate in SetScope or Unify... - if err := ast.Init(data); err != nil { + err = ast.Init(data) + if (!fail || !fail9) && err != nil { t.Errorf("test #%d: FAIL", index) t.Errorf("test #%d: could not init and validate AST: %+v", index, err) return } + if fail9 && err != nil { + // TODO: %+v instead? + s := fmt.Sprintf("%s", err) // convert to string + if s != expstr { + t.Errorf("test #%d: FAIL", index) + t.Errorf("test #%d: expected different error", index) + t.Logf("test #%d: err: %s", index, s) + t.Logf("test #%d: exp: %s", index, expstr) + } + return // fail happened during lex parse, don't run init/interpolate! + } + if fail9 && err == nil { + t.Errorf("test #%d: FAIL", index) + t.Errorf("test #%d: Init passed, expected fail", index) + return + } iast, err := ast.Interpolate() if err != nil { diff --git a/lang/interpret_test/TestAstFunc2/struct-duplicate0.output b/lang/interpret_test/TestAstFunc2/struct-duplicate0.output new file mode 100644 index 00000000..5d8fb049 --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/struct-duplicate0.output @@ -0,0 +1 @@ +# err: err1: parser: `syntax error: unexpected $end, expecting EQUALS` @1:6 diff --git a/lang/interpret_test/TestAstFunc2/struct-duplicate0/main.mcl b/lang/interpret_test/TestAstFunc2/struct-duplicate0/main.mcl new file mode 100644 index 00000000..71281ae3 --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/struct-duplicate0/main.mcl @@ -0,0 +1 @@ +$d []struct{x int;x int} diff --git a/lang/interpret_test/TestAstFunc2/struct-duplicate1.output b/lang/interpret_test/TestAstFunc2/struct-duplicate1.output new file mode 100644 index 00000000..45eb0d8b --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/struct-duplicate1.output @@ -0,0 +1 @@ +# err: err1: can't set return type in parser: `can't set return type in parser: duplicate struct field of `x int`` @1:24 diff --git a/lang/interpret_test/TestAstFunc2/struct-duplicate1/main.mcl b/lang/interpret_test/TestAstFunc2/struct-duplicate1/main.mcl new file mode 100644 index 00000000..3c697e79 --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/struct-duplicate1/main.mcl @@ -0,0 +1 @@ +$st struct{x int; x int} = struct{x => 0,} diff --git a/lang/interpret_test/TestAstFunc2/struct-duplicate2.output b/lang/interpret_test/TestAstFunc2/struct-duplicate2.output new file mode 100644 index 00000000..45eb0d8b --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/struct-duplicate2.output @@ -0,0 +1 @@ +# err: err1: can't set return type in parser: `can't set return type in parser: duplicate struct field of `x int`` @1:24 diff --git a/lang/interpret_test/TestAstFunc2/struct-duplicate2/main.mcl b/lang/interpret_test/TestAstFunc2/struct-duplicate2/main.mcl new file mode 100644 index 00000000..49c11139 --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/struct-duplicate2/main.mcl @@ -0,0 +1 @@ +$st struct{x int; x int} = struct{x => 0, x => 0,} diff --git a/lang/interpret_test/TestAstFunc2/struct-duplicate3.output b/lang/interpret_test/TestAstFunc2/struct-duplicate3.output new file mode 100644 index 00000000..f2ad69a3 --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/struct-duplicate3.output @@ -0,0 +1 @@ +# err: err9: duplicate struct field name of: `x` diff --git a/lang/interpret_test/TestAstFunc2/struct-duplicate3/main.mcl b/lang/interpret_test/TestAstFunc2/struct-duplicate3/main.mcl new file mode 100644 index 00000000..6931f4c9 --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/struct-duplicate3/main.mcl @@ -0,0 +1,2 @@ +$st struct{x str} = struct{x => "hello", x => "world",} +test structlookup($st, "x") {} diff --git a/lang/parser.y b/lang/parser.y index 97cf44dd..569a1d1f 100644 --- a/lang/parser.y +++ b/lang/parser.y @@ -48,8 +48,6 @@ func init() { int int64 // this is the .int as seen in lexer.nex float float64 - strSlice []string - typ *types.Type stmts []interfaces.Stmt @@ -1177,7 +1175,23 @@ type: // struct: struct{} or struct{a bool} or struct{a bool; bb int} { posLast(yylex, yyDollar) // our pos - $$.typ = types.NewType(fmt.Sprintf("%s{%s}", $1.str, strings.Join($3.strSlice, "; "))) + + names := make(map[string]struct{}) + strs := []string{} + for _, arg := range $3.args { + s := fmt.Sprintf("%s %s", arg.Name, arg.Type.String()) + if _, exists := names[s]; exists { + // duplicate field name used + err := fmt.Errorf("duplicate struct field of `%s`", s) + // this will ultimately cause a parser error to occur... + yylex.Error(fmt.Sprintf("%s: %+v", ErrParseSetType, err)) + break // we must skip, because code continues! + } + names[s] = struct{}{} + strs = append(strs, s) + } + + $$.typ = types.NewType(fmt.Sprintf("%s{%s}", $1.str, strings.Join(strs, "; "))) } | FUNC_IDENTIFIER OPEN_PAREN type_func_args CLOSE_PAREN type // XXX: should we allow named args in the type signature? @@ -1228,24 +1242,27 @@ type_struct_fields: /* end of list */ { posLast(yylex, yyDollar) // our pos - $$.strSlice = []string{} + $$.args = []*Arg{} } | type_struct_fields SEMICOLON type_struct_field { posLast(yylex, yyDollar) // our pos - $$.strSlice = append($1.strSlice, $3.str) + $$.args = append($1.args, $3.arg) } | type_struct_field { posLast(yylex, yyDollar) // our pos - $$.strSlice = []string{$1.str} + $$.args = append([]*Arg{}, $1.arg) } ; type_struct_field: IDENTIFIER type { posLast(yylex, yyDollar) // our pos - $$.str = fmt.Sprintf("%s %s", $1.str, $2.typ.String()) + $$.arg = &Arg{ // re-use the Arg struct + Name: $1.str, + Type: $2.typ, + } } ; type_func_args: diff --git a/lang/structs.go b/lang/structs.go index 62efb553..b80d11c5 100644 --- a/lang/structs.go +++ b/lang/structs.go @@ -5730,6 +5730,7 @@ func (obj *ExprMap) Apply(fn func(interfaces.Node) error) error { // Init initializes this branch of the AST, and returns an error if it fails to // validate. func (obj *ExprMap) Init(data *interfaces.Data) error { + // XXX: Can we check that there aren't any duplicate keys? Can we Cmp? for _, x := range obj.KVs { if err := x.Key.Init(data); err != nil { return err @@ -6252,7 +6253,14 @@ func (obj *ExprStruct) Apply(fn func(interfaces.Node) error) error { // Init initializes this branch of the AST, and returns an error if it fails to // validate. func (obj *ExprStruct) Init(data *interfaces.Data) error { + fields := make(map[string]struct{}) for _, x := range obj.Fields { + // Validate field names and ensure no duplicates! + if _, exists := fields[x.Name]; exists { + return fmt.Errorf("duplicate struct field name of: `%s`", x.Name) + } + fields[x.Name] = struct{}{} + if err := x.Value.Init(data); err != nil { return err }