Repository: VKCOM/noverify
Pull request #767 (2020-09-03 22:32:26)

ir/irconv: don't panic on invalid strings
Instead, emit BadString with a special Error field
that can be examined by the linter later to emit a warning.

php-parser does only partial strings validation, so
we might encounter badly encoded UTF-8 escapes as
well as some other issues. We'll report them as a
syntax errors in linter.

Previously, interpretString() function assumed that
parsed string literals are valid PHP literals.

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>


Patch:
diff --git a/src/ir/freefloating.go b/src/ir/freefloating.go
index f1c9bb1..46ebc3a 100644
--- a/src/ir/freefloating.go
+++ b/src/ir/freefloating.go
@@ -47,6 +47,8 @@ func (n *AssignShiftLeft) GetFreeFloating() *freefloating.Collection { return &n
 
 func (n *AssignShiftRight) GetFreeFloating() *freefloating.Collection { return &n.FreeFloating }
 
+func (n *BadString) GetFreeFloating() *freefloating.Collection { return &n.FreeFloating }
+
 func (n *BitwiseAndExpr) GetFreeFloating() *freefloating.Collection { return &n.FreeFloating }
 
 func (n *BitwiseNotExpr) GetFreeFloating() *freefloating.Collection { return &n.FreeFloating }
diff --git a/src/ir/get_node_kind.go b/src/ir/get_node_kind.go
index 8e96ab5..70c025e 100644
--- a/src/ir/get_node_kind.go
+++ b/src/ir/get_node_kind.go
@@ -29,6 +29,7 @@ const (
 	KindAssignReference
 	KindAssignShiftLeft
 	KindAssignShiftRight
+	KindBadString
 	KindBitwiseAndExpr
 	KindBitwiseNotExpr
 	KindBitwiseOrExpr
@@ -207,6 +208,8 @@ func GetNodeKind(x Node) NodeKind {
 		return KindAssignShiftLeft
 	case *AssignShiftRight:
 		return KindAssignShiftRight
+	case *BadString:
+		return KindBadString
 	case *BitwiseAndExpr:
 		return KindBitwiseAndExpr
 	case *BitwiseNotExpr:
diff --git a/src/ir/get_position.go b/src/ir/get_position.go
index c2e2924..8f9b017 100644
--- a/src/ir/get_position.go
+++ b/src/ir/get_position.go
@@ -50,6 +50,8 @@ func GetPosition(n Node) *position.Position {
 		return n.Position
 	case *AssignShiftRight:
 		return n.Position
+	case *BadString:
+		return n.Position
 	case *BitwiseAndExpr:
 		return n.Position
 	case *BitwiseNotExpr:
diff --git a/src/ir/irconv/irconv.go b/src/ir/irconv/irconv.go
index 9098492..b98b6f4 100644
--- a/src/ir/irconv/irconv.go
+++ b/src/ir/irconv/irconv.go
@@ -1803,7 +1803,7 @@ func (c *Converter) convClass(n *stmt.Class) ir.Node {
 	return out
 }
 
-func convString(n *scalar.String) *ir.String {
+func convString(n *scalar.String) ir.Node {
 	out := &ir.String{
 		FreeFloating: n.FreeFloating,
 		Position:     n.Position,
@@ -1820,10 +1820,16 @@ func convString(n *scalar.String) *ir.String {
 	}
 
 	out.DoubleQuotes = n.Value[0] == '"'
-	s, ok := interpretString(irutil.Unquote(n.Value), quote)
-	if !ok {
-		panic(fmt.Sprintf("can't interpret %s (line=%d bytes=%#v)",
-			n.Value, n.GetPosition().StartLine, []byte(n.Value)))
+	unquoted := irutil.Unquote(n.Value)
+	s, err := interpretString(unquoted, quote)
+	if err != nil {
+		return &ir.BadString{
+			FreeFloating: n.FreeFloating,
+			Position:     n.Position,
+			Value:        unquoted,
+			Error:        err.Error(),
+			DoubleQuotes: out.DoubleQuotes,
+		}
 	}
 	out.Value = s
 
diff --git a/src/ir/irconv/utils.go b/src/ir/irconv/utils.go
index 3a9cd90..8c7a9e5 100644
--- a/src/ir/irconv/utils.go
+++ b/src/ir/irconv/utils.go
@@ -1,6 +1,8 @@
 package irconv
 
 import (
+	"errors"
+	"fmt"
 	"strconv"
 	"strings"
 	"unicode"
@@ -34,17 +36,17 @@ func namePartsToString(parts []node.Node) string {
 //
 // If, for whatever reason, a bad strign escape was encountered,
 // second returned value will be false.
-func interpretString(s string, quote byte) (string, bool) {
+func interpretString(s string, quote byte) (string, error) {
 	switch quote {
 	case '\'', '"':
 		// OK
 	default:
-		return "", false
+		panic("invalid quote type")
 	}
 
 	if !strings.Contains(s, `\`) {
 		// Fast path: nothing to replace.
-		return s, true
+		return s, nil
 	}
 
 	// To understand what's going on, consult the manual:
@@ -57,7 +59,7 @@ func interpretString(s string, quote byte) (string, bool) {
 }
 
 // interpretStringQ1 returns s interpreted value as a single-quoted PHP string.
-func interpretStringQ1(s string) (string, bool) {
+func interpretStringQ1(s string) (string, error) {
 	var out strings.Builder
 	out.Grow(len(s))
 
@@ -68,7 +70,7 @@ func interpretStringQ1(s string) (string, bool) {
 		switch {
 		case ch == '\\':
 			if !hasOffset(s, i+1) {
-				return "", false
+				return "", errors.New(`illegal trailing \`)
 			}
 			switch s[i+1] {
 			case '\'':
@@ -93,11 +95,11 @@ func interpretStringQ1(s string) (string, bool) {
 		}
 	}
 
-	return out.String(), true
+	return out.String(), nil
 }
 
 // interpretStringQ2 returns s interpreted value as a double-quoted PHP string.
-func interpretStringQ2(s string) (string, bool) {
+func interpretStringQ2(s string) (string, error) {
 	var out strings.Builder
 	out.Grow(len(s))
 
@@ -108,7 +110,7 @@ func interpretStringQ2(s string) (string, bool) {
 		switch {
 		case ch == '\\':
 			if !hasOffset(s, i+1) {
-				return "", false
+				return "", errors.New(`illegal trailing \`)
 			}
 			switch s[i+1] {
 			case 'u': // \u{[0-9A-Fa-f]+}
@@ -119,13 +121,13 @@ func interpretStringQ2(s string) (string, bool) {
 				}
 				end := strings.IndexByte(s[i+len(`\u`):], '}')
 				if end == -1 {
-					return "", false
+					return "", errors.New("missing closing '}' for UTF-8 sequence")
 				}
 				codepoints := s[i+len(`\u{`) : i+len(`\u{`)+end-len(`}`)]
 				goLiteral := `\U` + zeros[:8-len(codepoints)] + codepoints
 				ch, _, _, err := strconv.UnquoteChar(goLiteral, '"')
 				if err != nil {
-					return "", false
+					return "", fmt.Errorf("decode UTF-8 codepoints: %v", err)
 				}
 				out.WriteRune(ch)
 				i += len(`\u{`) + len(codepoints) + len(`}`)
@@ -210,7 +212,7 @@ func interpretStringQ2(s string) (string, bool) {
 		}
 	}
 
-	return out.String(), true
+	return out.String(), nil
 }
 
 var zeros = "00000000"
diff --git a/src/ir/irutil/clone.go b/src/ir/irutil/clone.go
index 0496634..cd2902f 100644
--- a/src/ir/irutil/clone.go
+++ b/src/ir/irutil/clone.go
@@ -195,6 +195,9 @@ func NodeClone(x ir.Node) ir.Node {
 			clone.Expression = NodeClone(x.Expression)
 		}
 		return &clone
+	case *ir.BadString:
+		clone := *x
+		return &clone
 	case *ir.BitwiseAndExpr:
 		clone := *x
 		if x.Left != nil {
diff --git a/src/ir/irutil/equal.go b/src/ir/irutil/equal.go
index 2a67fb5..86a6704 100644
--- a/src/ir/irutil/equal.go
+++ b/src/ir/irutil/equal.go
@@ -286,6 +286,21 @@ func NodeEqual(x, y ir.Node) bool {
 			return false
 		}
 		return true
+	case *ir.BadString:
+		y, ok := y.(*ir.BadString)
+		if !ok || x == nil || y == nil {
+			return x == y
+		}
+		if x.Value != y.Value {
+			return false
+		}
+		if x.DoubleQuotes != y.DoubleQuotes {
+			return false
+		}
+		if x.Error != y.Error {
+			return false
+		}
+		return true
 	case *ir.BitwiseAndExpr:
 		y, ok := y.(*ir.BitwiseAndExpr)
 		if !ok || x == nil || y == nil {
diff --git a/src/ir/node_types.go b/src/ir/node_types.go
index d0f6407..84826b3 100644
--- a/src/ir/node_types.go
+++ b/src/ir/node_types.go
@@ -815,7 +815,7 @@ type MagicConstant struct {
 // The $Value contains interpreted string bytes, if you need a raw
 // string value, use positions and fetch relevant the source bytes.
 //
-// DoubleQuotes tell whether originally this string literal was ""-quoted.
+// $DoubleQuotes tell whether originally this string literal was ""-quoted.
 type String struct {
 	FreeFloating freefloating.Collection
 	Position     *position.Position
@@ -823,6 +823,17 @@ type String struct {
 	DoubleQuotes bool
 }
 
+// BadString is a string that we couldn't interpret correctly.
+// The $Value contains uninterpreted (raw) string bytes.
+// $Error contains the reason why this string is "bad".
+type BadString struct {
+	FreeFloating freefloating.Collection
+	Position     *position.Position
+	Value        string
+	DoubleQuotes bool
+	Error        string
+}
+
 // BreakStmt is a `break $Expr` statement.
 type BreakStmt struct {
 	FreeFloating freefloating.Collection
diff --git a/src/ir/walk.go b/src/ir/walk.go
index 270d062..e3616af 100644
--- a/src/ir/walk.go
+++ b/src/ir/walk.go
@@ -277,6 +277,13 @@ func (n *AssignShiftRight) Walk(v Visitor) {
 	v.LeaveNode(n)
 }
 
+func (n *BadString) Walk(v Visitor) {
+	if !v.EnterNode(n) {
+		return
+	}
+	v.LeaveNode(n)
+}
+
 func (n *BitwiseAndExpr) Walk(v Visitor) {
 	if !v.EnterNode(n) {
 		return
diff --git a/src/linter/block_linter.go b/src/linter/block_linter.go
index e376d66..7742192 100644
--- a/src/linter/block_linter.go
+++ b/src/linter/block_linter.go
@@ -163,6 +163,9 @@ func (b *blockLinter) enterNode(n ir.Node) {
 
 	case *ir.InterfaceStmt:
 		b.checkInterfaceStmt(n)
+
+	case *ir.BadString:
+		b.report(n, LevelSyntax, "syntax", "%s", n.Error)
 	}
 }
 


Test patch:
diff --git a/src/ir/irconv/irconv_test.go b/src/ir/irconv/irconv_test.go
index 8eaa932..bdd3ed5 100644
--- a/src/ir/irconv/irconv_test.go
+++ b/src/ir/irconv/irconv_test.go
@@ -2,6 +2,7 @@ package irconv
 
 import (
 	"bytes"
+	"strings"
 	"testing"
 )
 
@@ -251,12 +252,12 @@ func TestInterpretString(t *testing.T) {
 		{
 			raw:         `\u{zzzz}`,
 			singleQuote: []byte(`\u{zzzz}`),
-			doubleQuote: []byte("FAIL"),
+			doubleQuote: []byte("FAIL: decode UTF-8 codepoints: invalid syntax"),
 		},
 		{
 			raw:         `\u{zz`,
 			singleQuote: []byte(`\u{zz`),
-			doubleQuote: []byte("FAIL"),
+			doubleQuote: []byte("FAIL: missing closing '}' for UTF-8 sequence"),
 		},
 		{
 			raw:         `\u0`,
@@ -272,26 +273,33 @@ func TestInterpretString(t *testing.T) {
 		// Unterminated escape sequence.
 		{
 			raw:         `a\`,
-			singleQuote: []byte("FAIL"),
-			doubleQuote: []byte("FAIL"),
+			singleQuote: []byte(`FAIL: illegal trailing \`),
+			doubleQuote: []byte(`FAIL: illegal trailing \`),
 		},
 	}
 
 	runTest := func(t *testing.T, raw string, quote byte, want []byte) {
 		t.Helper()
-		have, ok := interpretString(raw, quote)
+		have, err := interpretString(raw, quote)
 
-		if bytes.Equal(want, []byte("FAIL")) {
-			if ok {
+		if wantString := string(want); strings.HasPrefix(wantString, "FAIL: ") {
+			wantString = strings.TrimPrefix(wantString, "FAIL: ")
+			if err == nil {
 				t.Errorf("expected %s interpretation to fail, got success", raw)
+				return
+			}
+			if wantString != err.Error() {
+				t.Errorf("interpretation error mismatch:\nhave: %s\nwant: %s", err.Error(), wantString)
+				return
 			}
 			return
 		}
 
-		if !ok {
-			t.Errorf("failed to interpret %s (quote=%c)", raw, quote)
+		if err != nil {
+			t.Errorf("failed to interpret %s (quote=%c): %v", raw, quote, err)
 			return
 		}
+
 		haveBytes := []byte(have)
 		if !bytes.Equal(haveBytes, want) {
 			t.Errorf("%s bytes mismatch (quote=%c):\nhave: %[3]v\nwant: %[4]v\nhave-string: %[3]s\nwant-string: %[4]s",
diff --git a/src/tests/checkers/basic_test.go b/src/tests/checkers/basic_test.go
index ee1b263..09cc1da 100644
--- a/src/tests/checkers/basic_test.go
+++ b/src/tests/checkers/basic_test.go
@@ -12,6 +12,19 @@ import (
 	"github.com/VKCOM/noverify/src/meta"
 )
 
+func TestBadString(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+$_ = "\u{";
+$_ = "\u{zzzz}";
+`)
+	test.Expect = []string{
+		`missing closing '}' for UTF-8 sequence`,
+		`decode UTF-8 codepoints: invalid syntax`,
+	}
+	test.RunAndMatch()
+}
+
 func TestStringNoQuotes(t *testing.T) {
 	linttest.SimpleNegativeTest(t, `<?php
 $arr = [];