Repository: VKCOM/noverify
Pull request #697 (2020-08-21 07:52:14)

linter: add paramClobber checker
Finds cases where function/method param is overwritten
before being used.

We give up if we encounter func_get_args() as there is
a pattern where named arguments are replaced with
func_get_args elements, like so:

```php
	function f($x, $y) {
		$args = func_get_args();
		$x = $args[0];
		$y = $args[1];
		// ...
	}
```

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>


Patch:
diff --git a/src/linter/block.go b/src/linter/block.go
index c2e64de..438faab 100644
--- a/src/linter/block.go
+++ b/src/linter/block.go
@@ -80,6 +80,8 @@ type BlockWalker struct {
 	// whether a function has a return with explicit expression.
 	// When can't infer precise type, can use mixed.
 	returnsValue bool
+	// whether func_get_args() was called.
+	callsFuncGetArgs bool
 
 	// callsParentConstructor is set to true when parent::__construct() call
 	// is found. This is needed for a root walker to report constructors
@@ -87,7 +89,8 @@ type BlockWalker struct {
 	callsParentConstructor bool
 
 	// shared state between all blocks
-	unusedVars map[string][]ir.Node
+	unusedVars   map[string][]ir.Node
+	unusedParams map[string]struct{}
 
 	// static, global and other vars that have complex control flow.
 	// Never contains varLocal elements.
@@ -410,7 +413,7 @@ func (b *BlockWalker) replaceVar(v ir.Node, typ meta.TypesMap, reason string, fl
 func (b *BlockWalker) trackVarName(n ir.Node, nm string) {
 	// Writes to non-local variables do count as usages
 	if _, ok := b.nonLocalVars[nm]; ok {
-		delete(b.unusedVars, nm)
+		b.untrackVarName(nm)
 		return
 	}
 
@@ -421,6 +424,11 @@ func (b *BlockWalker) trackVarName(n ir.Node, nm string) {
 	}
 }
 
+func (b *BlockWalker) untrackVarName(nm string) {
+	delete(b.unusedVars, nm)
+	delete(b.unusedParams, nm)
+}
+
 func (b *BlockWalker) addVarName(n ir.Node, nm string, typ meta.TypesMap, reason string, flags meta.VarFlags) {
 	b.ctx.sc.AddVarName(nm, typ, reason, flags)
 	b.trackVarName(n, nm)
@@ -465,7 +473,7 @@ func (b *BlockWalker) handleUnset(s *ir.UnsetStmt) bool {
 	for _, v := range s.Vars {
 		switch v := v.(type) {
 		case *ir.SimpleVar:
-			delete(b.unusedVars, v.Name)
+			b.untrackVarName(v.Name)
 			b.ctx.sc.DelVar(v, "unset")
 		case *ir.Var:
 			b.ctx.sc.DelVar(v, "unset")
@@ -487,7 +495,7 @@ func (b *BlockWalker) handleIsset(s *ir.IssetExpr) bool {
 		case *ir.Var:
 			// Do nothing.
 		case *ir.SimpleVar:
-			delete(b.unusedVars, v.Name)
+			b.untrackVarName(v.Name)
 		case *ir.ArrayDimFetchExpr:
 			b.handleIssetDimFetch(v)
 		default:
@@ -505,7 +513,7 @@ func (b *BlockWalker) handleEmpty(s *ir.EmptyExpr) bool {
 	case *ir.Var:
 		// Do nothing.
 	case *ir.SimpleVar:
-		delete(b.unusedVars, v.Name)
+		b.untrackVarName(v.Name)
 	case *ir.ArrayDimFetchExpr:
 		b.handleIssetDimFetch(v)
 	default:
@@ -629,7 +637,7 @@ func (b *BlockWalker) handleIssetDimFetch(e *ir.ArrayDimFetchExpr) {
 
 	switch v := e.Variable.(type) {
 	case *ir.SimpleVar:
-		delete(b.unusedVars, v.Name)
+		b.untrackVarName(v.Name)
 	case *ir.ArrayDimFetchExpr:
 		b.handleIssetDimFetch(v)
 	default:
@@ -742,6 +750,10 @@ func (b *BlockWalker) handleFunctionCall(e *ir.FunctionCallExpr) bool {
 
 	e.Function.Walk(b)
 
+	if call.fqName == `\func_get_args` {
+		b.callsFuncGetArgs = true
+	}
+
 	if call.fqName == `\compact` {
 		b.handleCompactCallArgs(e.Args)
 	} else {
@@ -1102,7 +1114,7 @@ func (b *BlockWalker) handleForeach(s *ir.ForeachStmt) bool {
 			return false
 		}
 
-		delete(b.unusedVars, key.Name)
+		b.untrackVarName(key.Name)
 
 		b.r.Report(s.Key, LevelError, "unused", "foreach key $%s is unused, can simplify $%s => $%s to just $%s", key.Name, key.Name, variable.Name, variable.Name)
 	}
@@ -1178,7 +1190,7 @@ func (b *BlockWalker) enterClosure(fun *ir.ClosureExpr, haveThis bool, thisType
 			sc.AddVarName(v.Name, typ, "use", meta.VarAlwaysDefined)
 		}
 
-		delete(b.unusedVars, v.Name)
+		b.untrackVarName(v.Name)
 	}
 
 	params, _ := b.r.parseFuncArgs(fun.Params, phpDocParamTypes, sc)
@@ -1268,10 +1280,10 @@ func (b *BlockWalker) handleVariable(v ir.Node) bool {
 	switch v := v.(type) {
 	case *ir.Var:
 		if vv, ok := v.Expr.(*ir.SimpleVar); ok {
-			delete(b.unusedVars, vv.Name)
+			b.untrackVarName(vv.Name)
 		}
 	case *ir.SimpleVar:
-		delete(b.unusedVars, v.Name)
+		b.untrackVarName(v.Name)
 	}
 
 	if !b.ctx.sc.HaveVar(v) {
@@ -1635,6 +1647,15 @@ func (b *BlockWalker) handleAssignList(items []*ir.ArrayItemExpr, info meta.Clas
 	}
 }
 
+func (b *BlockWalker) paramClobberCheck(v *ir.SimpleVar) {
+	if b.callsFuncGetArgs {
+		return
+	}
+	if _, ok := b.unusedParams[v.Name]; ok && !b.path.Conditional() {
+		b.r.Report(v, LevelWarning, "paramClobber", "$%s param re-assigned before being used", v.Name)
+	}
+}
+
 func (b *BlockWalker) handleAssign(a *ir.Assign) bool {
 	a.Expression.Walk(b)
 
@@ -1643,7 +1664,10 @@ func (b *BlockWalker) handleAssign(a *ir.Assign) bool {
 		typ := solver.ExprTypeLocal(b.ctx.sc, b.r.ctx.st, a.Expression)
 		b.handleDimFetchLValue(v, "assign_array", typ)
 		return false
-	case *ir.Var, *ir.SimpleVar:
+	case *ir.SimpleVar:
+		b.paramClobberCheck(v)
+		b.replaceVar(v, solver.ExprTypeLocal(b.ctx.sc, b.r.ctx.st, a.Expression), "assign", meta.VarAlwaysDefined)
+	case *ir.Var:
 		b.replaceVar(v, solver.ExprTypeLocal(b.ctx.sc, b.r.ctx.st, a.Expression), "assign", meta.VarAlwaysDefined)
 	case *ir.ListExpr:
 		if !meta.IsIndexingComplete() {
@@ -1675,7 +1699,7 @@ func (b *BlockWalker) handleAssign(a *ir.Assign) bool {
 			break
 		}
 
-		delete(b.unusedVars, sv.Name)
+		b.untrackVarName(sv.Name)
 
 		if sv.Name != "this" {
 			break
diff --git a/src/linter/root.go b/src/linter/root.go
index 8ef7958..153cd90 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -629,10 +629,18 @@ func (d *RootWalker) handleFuncStmts(params []meta.FuncParam, uses, stmts []ir.N
 		}
 	}
 
+	// It's OK to read from and delete from a nil map.
+	// If a func/method has 0 params, don't allocate a map for it.
+	if len(params) != 0 {
+		b.unusedParams = make(map[string]struct{}, len(params))
+	}
 	for _, p := range params {
 		if p.IsRef {
 			b.nonLocalVars[p.Name] = varRef
 		}
+		if !p.IsRef && !IsDiscardVar(p.Name) {
+			b.unusedParams[p.Name] = struct{}{}
+		}
 	}
 	for _, s := range stmts {
 		b.addStatement(s)


Test patch:
diff --git a/src/tests/checkers/basic_test.go b/src/tests/checkers/basic_test.go
index 3aec5d8..530d345 100644
--- a/src/tests/checkers/basic_test.go
+++ b/src/tests/checkers/basic_test.go
@@ -1267,6 +1267,7 @@ func TestForeachByRefUnused(t *testing.T) {
 	 * @param SomeClass[] $some_arr
 	 */
 	function doSometing($some_arr) {
+		$_ = $some_arr;
 		$some_arr = [];
 
 		foreach ($some_arr as $var) {
diff --git a/src/tests/checkers/paramClobber_test.go b/src/tests/checkers/paramClobber_test.go
new file mode 100644
index 0000000..e80a21d
--- /dev/null
+++ b/src/tests/checkers/paramClobber_test.go
@@ -0,0 +1,94 @@
+package checkers_test
+
+import (
+	"testing"
+
+	"github.com/VKCOM/noverify/src/linttest"
+)
+
+func TestParamClobberLegacyVariadic(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+function f($x, $y) {
+  $args = func_get_args();
+  $x = $args[0];
+  $y = $args[1];
+  return [$x, $y];
+}
+`)
+}
+
+func TestParamClobberReferenced(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+function f(array $x) {
+  $x = $x['foo'];
+  return $x;
+}
+`)
+}
+
+func TestParamClobberConditional(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+function f1($x, $y) {
+  if ($y) {
+    $x = 10;
+  }
+  return $x + $y;
+}
+
+function f2($x) {
+  try {
+    echo 123;
+  } catch (Exception $_) {
+    $x = "failed";
+  }
+  return $x;
+}
+`)
+}
+
+func TestParamClobberFunc(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+function f($x) {
+  $x = 1343;
+  return $x;
+}
+`)
+	test.Expect = []string{
+		`$x param re-assigned before being used`,
+	}
+	test.RunAndMatch()
+}
+
+func TestParamClobberMethod(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+class C {
+  /** f is an example method */
+  public function f($x) {
+    $x = 1343;
+    return $x;
+  }
+}
+`)
+	test.Expect = []string{
+		`$x param re-assigned before being used`,
+	}
+	test.RunAndMatch()
+}
+
+func TestParamClobberClosure(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+function f() {
+  return function ($x) {
+    $x = 1343;
+    return $x;
+  };
+}
+`)
+	test.Expect = []string{
+		`$x param re-assigned before being used`,
+	}
+	test.RunAndMatch()
+}