Repository: VKCOM/noverify
Pull request #701 (2020-08-22 16:31:58)

linter: add printf checker
Check printf and sprintf functions format string argument.

* Detects invalid argument references (too few args)
* Detects unused arguments (too much args)
* Detects some invalid fmt arguments

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>


Patch:
diff --git a/src/linter/block_linter.go b/src/linter/block_linter.go
index 2138e34..cf25d51 100644
--- a/src/linter/block_linter.go
+++ b/src/linter/block_linter.go
@@ -578,6 +578,12 @@ func (b *blockLinter) checkFunctionCall(e *ir.FunctionCallExpr) {
 			break
 		}
 		b.checkRegexp(e, e.Arg(0))
+	case `\sprintf`, `\printf`:
+		if len(e.Args) < 1 {
+			break
+		}
+		// TODO: handle fprintf as well?
+		b.checkFormatString(e, e.Arg(0))
 	}
 }
 
@@ -624,3 +630,66 @@ func (b *blockLinter) checkRegexp(e *ir.FunctionCallExpr, arg *ir.Argument) {
 		b.report(arg, LevelWarning, "regexpVet", "%s", issue)
 	}
 }
+
+func (b *blockLinter) checkFormatString(e *ir.FunctionCallExpr, arg *ir.Argument) {
+	s, ok := arg.Expr.(*ir.String)
+	if !ok {
+		return
+	}
+	const argsLimit = 16
+	if len(s.Value) > 255 || len(e.Args) > argsLimit {
+		return
+	}
+
+	format, err := parseFormatString(s.Value)
+	if err != nil {
+		b.report(arg, LevelWarning, "printf", "%s", err.Error())
+		return
+	}
+
+	// TODO: detect `% <char>` cases.
+	// For example in, "Handler % tried to add additional_field %s but % could not be added!"
+	// we have 2 bad formatting directives here, but only one is reported, `% t`, since
+	// 't' is not a correct specifier (while '% c' is technically OK).
+	//
+	// TODO: test whether things like `%1%` make sense. We report all %% directive
+	// usages that have any modifiers.
+
+	usages := make([]uint8, argsLimit)
+	for _, d := range format.directives {
+		if d.specifier == '%' {
+			hasModifiers := d.argNum != -1 || d.flags != "" || d.precision != -1 || d.width != -1
+			if hasModifiers {
+				b.report(arg, LevelWarning, "printf", "%%%% directive has modifiers")
+			}
+			continue
+		}
+
+		if d.argNum == -1 {
+			continue
+		}
+		if d.argNum >= len(e.Args) {
+			s := s.Value[d.begin:d.end]
+			b.report(arg, LevelWarning, "printf", "%s directive refers to the args[%d] which is not provided", s, d.argNum)
+			continue
+		}
+		if d.argNum < len(usages) {
+			usages[d.argNum]++
+		}
+
+		arg := e.Arg(d.argNum)
+		if d.specifier == 's' && b.isArrayType(b.walker.exprType(arg.Expr)) {
+			b.report(arg, LevelWarning, "printf", "potential array to string conversion")
+		}
+	}
+
+	for i := 1; i < len(e.Args); i++ {
+		if usages[i] == 0 {
+			b.report(e.Arg(i), LevelWarning, "printf", "argument is not referenced from the formatting string")
+		}
+	}
+}
+
+func (b *blockLinter) isArrayType(typ meta.TypesMap) bool {
+	return typ.Len() == 1 && typ.Find(meta.IsArrayType)
+}
diff --git a/src/linter/fmtstring.go b/src/linter/fmtstring.go
new file mode 100644
index 0000000..52d5032
--- /dev/null
+++ b/src/linter/fmtstring.go
@@ -0,0 +1,245 @@
+package linter
+
+import (
+	"errors"
+	"fmt"
+	"strconv"
+	"strings"
+)
+
+type fmtDirective struct {
+	begin uint
+	end   uint
+
+	// specifier is a verb-like character.
+	// Examples: `d` `s`
+	specifier byte
+
+	explicitArgNum bool
+
+	// An argNum of -1 is used when the directive does not refer to any argument.
+	argNum int
+
+	// flags is a string that combines all directive flags.
+	// Examples: `+` `'.`
+	flags string
+
+	// precision controls the specifier formatting (the exact effect depends on the specifier).
+	// A precision of -1 means that this directive had no explicit precision specifier.
+	// Examples: `.4` `.`
+	precision int
+
+	// width says how many characters (minimum) this conversion should result in.
+	// A width of -1 means that this directive had no explicit width specifier.
+	width int
+}
+
+func (d fmtDirective) String() string {
+	var parts []string
+
+	parts = append(parts, fmt.Sprintf("%%%c", d.specifier))
+
+	if d.argNum != -1 {
+		parts = append(parts, fmt.Sprintf("arg=%d", d.argNum))
+		if !d.explicitArgNum {
+			parts = append(parts, "(implicit)")
+		}
+	}
+
+	if d.precision != -1 {
+		parts = append(parts, fmt.Sprintf("p=%d", d.precision))
+	}
+	if d.width != -1 {
+		parts = append(parts, fmt.Sprintf("w=%d", d.width))
+	}
+
+	if d.flags != "" {
+		parts = append(parts, fmt.Sprintf("flags=%s", d.flags))
+	}
+
+	return strings.Join(parts, " ")
+}
+
+type fmtString struct {
+	directives []fmtDirective
+}
+
+func parseFormatString(s string) (fmtString, error) {
+	var p fmtStringParser
+	return p.Parse(s)
+}
+
+type fmtStringParser struct {
+	input      string
+	pos        uint
+	directives []fmtDirective
+
+	argNum int
+	d      fmtDirective
+}
+
+func (p *fmtStringParser) Parse(s string) (fmtString, error) {
+	p.reset(s)
+
+	err := p.parse()
+
+	result := fmtString{
+		directives: p.directives,
+	}
+
+	return result, err
+}
+
+func (p *fmtStringParser) reset(s string) {
+	p.input = s
+	p.pos = 0
+	p.argNum = 1
+}
+
+func (p *fmtStringParser) peek(offset uint) byte {
+	pos := p.pos + offset
+	if pos < uint(len(p.input)) {
+		return p.input[pos]
+	}
+	return 0
+}
+
+func (p *fmtStringParser) parse() error {
+	for p.pos < uint(len(p.input)) {
+		if p.peek(0) != '%' {
+			p.pos++
+			continue
+		}
+		if err := p.parseDirective(); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (p *fmtStringParser) parseDirective() error {
+	p.d = fmtDirective{
+		begin:     p.pos,
+		width:     -1,
+		precision: -1,
+	}
+	p.pos += uint(len("%"))
+
+	p.parseArgnum()
+	if err := p.parseFlags(); err != nil {
+		return err
+	}
+	p.parseWidth()
+	p.parsePrecision()
+	if err := p.parseSpecifier(); err != nil {
+		return err
+	}
+
+	p.d.end = p.pos
+	p.directives = append(p.directives, p.d)
+	return nil
+}
+
+func (p *fmtStringParser) parseArgnum() {
+	digits := p.scanDigits()
+	if digits == "" {
+		return
+	}
+	if p.peek(uint(len(digits))) != '$' {
+		return
+	}
+	p.d.argNum = p.atoi(digits)
+	p.d.explicitArgNum = true
+	p.pos += uint(len(digits) + len("$"))
+}
+
+func (p *fmtStringParser) parseFlags() error {
+	length := uint(0)
+loop:
+	for {
+		switch ch := p.peek(length); ch {
+		case '-', '+', ' ', '0':
+			length++
+		case '\'':
+			if p.peek(length+1) == 0 {
+				return errors.New("'-flag expects a char, found end of string")
+			}
+			length += 2
+		default:
+			break loop
+		}
+	}
+
+	p.d.flags = p.input[p.pos : p.pos+length]
+	p.pos += length
+	return nil
+}
+
+func (p *fmtStringParser) parseWidth() {
+	digits := p.scanDigits()
+	if digits == "" {
+		return
+	}
+	p.d.width = p.atoi(digits)
+	p.pos += uint(len(digits))
+}
+
+func (p *fmtStringParser) parsePrecision() {
+	if p.peek(0) != '.' {
+		return
+	}
+	p.pos += uint(len("."))
+	digits := p.scanDigits()
+	if digits == "" {
+		return // Not a parse error, default is implied
+	}
+	p.d.precision = p.atoi(digits)
+	p.pos += uint(len(digits))
+}
+
+func (p *fmtStringParser) parseSpecifier() error {
+	ch := p.peek(0)
+	switch ch {
+	case 'b', 'c', 'd', 'e', 'E', 'f', 'F', 'g', 'G', 'o', 's', 'u', 'x', 'X':
+		if !p.d.explicitArgNum {
+			p.d.argNum = p.argNum
+			p.argNum++
+		}
+	case '%':
+		// Does not increment the argnum.
+		if !p.d.explicitArgNum {
+			p.d.argNum = -1
+		}
+	default:
+		return fmt.Errorf("unexpected format specifier %c", ch)
+	}
+	p.d.specifier = ch
+	p.pos++
+	return nil
+}
+
+func (p *fmtStringParser) scanDigits() string {
+	length := uint(0)
+	for p.isDigit(p.peek(length)) {
+		length++
+	}
+	if length == 0 {
+		return ""
+	}
+	return p.input[p.pos : p.pos+length]
+}
+
+func (p *fmtStringParser) atoi(digits string) int {
+	v, err := strconv.Atoi(digits)
+	if err != nil {
+		// Since digits string contains only decimal chars
+		// atoi call should never fail unless we get something
+		// wrong during the input slicing.
+		panic(fmt.Sprintf("unexpected digits='%s' parse failure", digits))
+	}
+	return v
+}
+
+func (p *fmtStringParser) isDigit(ch byte) bool {
+	return ch >= '0' && ch <= '9'
+}
diff --git a/src/linter/report.go b/src/linter/report.go
index dd33ae3..2eace47 100644
--- a/src/linter/report.go
+++ b/src/linter/report.go
@@ -510,6 +510,14 @@ $foo = new Foo();`,
   return get_video($user_id);
 }`,
 		},
+
+		{
+			Name:    "printf",
+			Default: true,
+			Comment: `Report issues in printf-like function calls.`,
+			Before:  `sprintf("id=%d")`,
+			After:   `sprintf("id=%d", $id)`,
+		},
 	}
 
 	for _, info := range allChecks {


Test patch:
diff --git a/src/linter/fmtstring_test.go b/src/linter/fmtstring_test.go
new file mode 100644
index 0000000..4d6a8b4
--- /dev/null
+++ b/src/linter/fmtstring_test.go
@@ -0,0 +1,95 @@
+package linter
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestParseFormatString(t *testing.T) {
+	tests := []struct {
+		input      string
+		directives []string
+	}{
+		{input: ``, directives: []string{}},
+		{input: `foo`, directives: []string{}},
+
+		{
+			input: `foo%d%0$fbar`,
+			directives: []string{
+				`%d arg=1 (implicit)`,
+				`%f arg=0`,
+			},
+		},
+
+		{
+			input: `%10.f%.5f`,
+			directives: []string{
+				`%f arg=1 (implicit) w=10`,
+				`%f arg=2 (implicit) p=5`,
+			},
+		},
+
+		{
+			input: ` %10.f %.5f `,
+			directives: []string{
+				`%f arg=1 (implicit) w=10`,
+				`%f arg=2 (implicit) p=5`,
+			},
+		},
+
+		{
+			input: ` %10$+-+500.71g `,
+			directives: []string{
+				`%g arg=10 p=71 w=500 flags=+-+`,
+			},
+		},
+
+		{
+			input: `%10$+-+500g`,
+			directives: []string{
+				`%g arg=10 w=500 flags=+-+`,
+			},
+		},
+
+		// Explicit arg nums do not increment the arg counter.
+		{
+			input: `%1$d %1d %d`,
+			directives: []string{
+				"%d arg=1",
+				"%d arg=1 (implicit) w=1",
+				"%d arg=2 (implicit)",
+			},
+		},
+
+		// %% does not increment the arg counter.
+		{
+			input: `%% %4$% %d %.5% %d`,
+			directives: []string{
+				"%%",
+				"%% arg=4",
+				"%d arg=1 (implicit)",
+				"%% p=5",
+				"%d arg=2 (implicit)",
+			},
+		},
+	}
+
+	for _, test := range tests {
+		parsed, err := parseFormatString(test.input)
+		if err != nil {
+			t.Errorf("parse `%s`: %v", test.input, err)
+			continue
+		}
+		directives := make([]string, len(parsed.directives))
+		for i, d := range parsed.directives {
+			directives[i] = d.String()
+		}
+		wantDirectives := test.directives
+		haveDirectives := directives
+		if diff := cmp.Diff(wantDirectives, haveDirectives); diff != "" {
+			t.Errorf("directives mismatch (+ have) (- want): %s", diff)
+			continue
+		}
+	}
+}
diff --git a/src/tests/checkers/printf_test.go b/src/tests/checkers/printf_test.go
new file mode 100644
index 0000000..47e5b62
--- /dev/null
+++ b/src/tests/checkers/printf_test.go
@@ -0,0 +1,71 @@
+package checkers_test
+
+import (
+	"testing"
+
+	"github.com/VKCOM/noverify/src/linttest"
+)
+
+func TestSprintf(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.LoadStubs = []string{`stubs/phpstorm-stubs/standard/standard_2.php`}
+	test.AddFile(`<?php
+function f($s, array $a) {
+  $_ = sprintf('%d');
+  $_ = sprintf('%2$d', 10); // arg not referenced
+  $_ = sprintf('foo%sbar%s', $s);
+  $_ = sprintf('%.2%');
+  $_ = sprintf('%z', $s);
+  $_ = sprintf("%'");
+
+  $ints = [1, 2];
+  $_ = sprintf("--%s--", $a);
+  $_ = sprintf("%s", $ints);
+  $_ = sprintf("%d", $ints);
+}
+`)
+	test.Expect = []string{
+		`%d directive refers to the args[1] which is not provided`,
+		`%2$d directive refers to the args[2] which is not provided`,
+		`%s directive refers to the args[2] which is not provided`,
+		`argument is not referenced from the formatting string`,
+		`%% directive has modifiers`,
+		`unexpected format specifier z`,
+		`'-flag expects a char, found end of string`,
+		`potential array to string conversion`,
+		`potential array to string conversion`,
+	}
+	test.RunAndMatch()
+}
+
+func TestPrintf(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.LoadStubs = []string{`stubs/phpstorm-stubs/standard/standard_2.php`}
+	test.AddFile(`<?php
+function f($s, array $a) {
+  printf('%d');
+  printf('%2$d', 10); // arg not referenced
+  printf('foo%sbar%s', $s);
+  printf('%.2%');
+  printf('%z', $s);
+  printf("%'");
+
+  $ints = [1, 2];
+  printf("--%s--", $a);
+  printf("%s", $ints);
+  printf("%d", $ints);
+}
+`)
+	test.Expect = []string{
+		`%d directive refers to the args[1] which is not provided`,
+		`%2$d directive refers to the args[2] which is not provided`,
+		`%s directive refers to the args[2] which is not provided`,
+		`argument is not referenced from the formatting string`,
+		`%% directive has modifiers`,
+		`unexpected format specifier z`,
+		`'-flag expects a char, found end of string`,
+		`potential array to string conversion`,
+		`potential array to string conversion`,
+	}
+	test.RunAndMatch()
+}