Skip to content

Commit ca1f8d4

Browse files
alban-auzeillpynicolas
alban-auzeill
authored and
pynicolas
committed
SONARPHP-752 Rule S836: Variables should be initialized before use (#254)
* SONARPHP-752 Rule S836: Variables should be initialized before use * Fix from review
1 parent 362ea43 commit ca1f8d4

File tree

9 files changed

+634
-1
lines changed

9 files changed

+634
-1
lines changed

‎its/ruling/src/test/resources/expected/php-S836.json

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
{
2+
'project:PHPExcel_1.7.9_doc/Classes/PHPExcel/Shared/JAMA/Matrix.php':[
3+
896,
4+
],
5+
'project:PHPExcel_1.7.9_doc/Classes/PHPExcel/Style.php':[
6+
408,
7+
],
8+
'project:symfony/Symfony/Component/Debug/Tests/ErrorHandlerTest.php':[
9+
159,
10+
159,
11+
],
12+
}

‎php-checks/src/main/java/org/sonar/php/checks/CheckList.java

+1
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ public static List<Class> getChecks() {
158158
NestedTernaryOperatorsCheck.class,
159159
DuplicateBranchImplementationCheck.class,
160160
AllBranchesIdenticalCheck.class,
161+
UseOfUninitializedVariableCheck.class,
161162
InterfaceNameCheck.class,
162163
CatchRethrowingCheck.class,
163164
CallToIniSetCheck.class,

‎php-checks/src/main/java/org/sonar/php/checks/IgnoredReturnValueCheck.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
@Rule(key = "S2201")
3838
public class IgnoredReturnValueCheck extends PHPVisitorCheck {
3939

40-
private static final Set<String> PURE_FUNCTIONS = new HashSet<>(Arrays.asList(
40+
protected static final Set<String> PURE_FUNCTIONS = new HashSet<>(Arrays.asList(
4141
// String Functions
4242
"addcslashes",
4343
"addslashes",

‎php-checks/src/main/java/org/sonar/php/checks/UseOfUninitializedVariableCheck.java

+270
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,270 @@
1+
/*
2+
* SonarQube PHP Plugin
3+
* Copyright (C) 2010-2017 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.php.checks;
21+
22+
import java.util.Arrays;
23+
import java.util.EnumMap;
24+
import java.util.EnumSet;
25+
import java.util.HashMap;
26+
import java.util.HashSet;
27+
import java.util.Locale;
28+
import java.util.Map;
29+
import java.util.Set;
30+
import java.util.function.Predicate;
31+
import java.util.stream.Stream;
32+
import javax.annotation.Nullable;
33+
import org.sonar.check.Rule;
34+
import org.sonar.php.checks.utils.CheckUtils;
35+
import org.sonar.plugins.php.api.tree.Tree;
36+
import org.sonar.plugins.php.api.tree.Tree.Kind;
37+
import org.sonar.plugins.php.api.tree.declaration.ClassDeclarationTree;
38+
import org.sonar.plugins.php.api.tree.declaration.FunctionDeclarationTree;
39+
import org.sonar.plugins.php.api.tree.declaration.FunctionTree;
40+
import org.sonar.plugins.php.api.tree.declaration.MethodDeclarationTree;
41+
import org.sonar.plugins.php.api.tree.declaration.VariableDeclarationTree;
42+
import org.sonar.plugins.php.api.tree.expression.AnonymousClassTree;
43+
import org.sonar.plugins.php.api.tree.expression.ArrayAccessTree;
44+
import org.sonar.plugins.php.api.tree.expression.AssignmentExpressionTree;
45+
import org.sonar.plugins.php.api.tree.expression.FunctionCallTree;
46+
import org.sonar.plugins.php.api.tree.expression.FunctionExpressionTree;
47+
import org.sonar.plugins.php.api.tree.expression.LexicalVariablesTree;
48+
import org.sonar.plugins.php.api.tree.expression.MemberAccessTree;
49+
import org.sonar.plugins.php.api.tree.expression.VariableIdentifierTree;
50+
import org.sonar.plugins.php.api.tree.expression.VariableTree;
51+
import org.sonar.plugins.php.api.tree.statement.ForEachStatementTree;
52+
import org.sonar.plugins.php.api.visitors.PHPVisitorCheck;
53+
54+
@Rule(key = "S836")
55+
public class UseOfUninitializedVariableCheck extends PHPVisitorCheck {
56+
57+
private static final Set<Kind> PARENT_INITIALIZATION_KIND = EnumSet.of(
58+
// Note: LEXICAL_VARIABLES are both, read and write, see VariableVisitor#visitFunctionExpression
59+
Kind.PARAMETER,
60+
Kind.GLOBAL_STATEMENT,
61+
Kind.VARIABLE_DECLARATION,
62+
Kind.REFERENCE_VARIABLE,
63+
Kind.ARRAY_ASSIGNMENT_PATTERN_ELEMENT,
64+
Kind.UNSET_VARIABLE_STATEMENT,
65+
// CatchBlockTree#variable
66+
Kind.CATCH_BLOCK,
67+
Kind.ASSIGNMENT_BY_REFERENCE);
68+
69+
private static final Set<String> FUNCTION_CHANGING_CURRENT_SCOPE = new HashSet<>(Arrays.asList(
70+
"eval",
71+
"extract",
72+
"parse_str",
73+
// PREG_REPLACE_EVAL option was deprecated in php 5.5 and has been removed in php 7.0
74+
"preg_replace",
75+
"include",
76+
"include_once",
77+
"require",
78+
"require_once"));
79+
80+
// Note: "$argc" and "$argv" are not available in the function scope without using "global"
81+
private static final Set<String> PREDEFINED_VARIABLES = new HashSet<>(Arrays.asList(
82+
"$_COOKIE",
83+
"$_ENV",
84+
"$_FILES",
85+
"$_GET",
86+
"$_POST",
87+
"$_REQUEST",
88+
"$_SERVER",
89+
"$_SESSION",
90+
"$GLOBALS",
91+
"$HTTP_RAW_POST_DATA",
92+
"$http_response_header",
93+
"$php_errormsg",
94+
// "$this" is defined only in method, but rule S2014 raises issues when it's used elsewhere
95+
"$this"));
96+
97+
private static final Set<String> FUNCTION_ALLOWING_ARGUMENT_CHECK;
98+
static {
99+
FUNCTION_ALLOWING_ARGUMENT_CHECK = new HashSet<>(IgnoredReturnValueCheck.PURE_FUNCTIONS);
100+
FUNCTION_ALLOWING_ARGUMENT_CHECK.remove("isset");
101+
}
102+
103+
private static final Map<Kind, Predicate<Tree>> IS_READ_ACCESS_BY_PARENT_KIND = initializeReadPredicate();
104+
105+
private static Map<Kind, Predicate<Tree>> initializeReadPredicate() {
106+
Map<Kind, Predicate<Tree>> map = new EnumMap<>(Kind.class);
107+
108+
PARENT_INITIALIZATION_KIND.forEach(kind -> map.put(kind, tree -> false));
109+
map.put(Kind.ASSIGNMENT, tree -> tree == ((AssignmentExpressionTree) tree.getParent()).value());
110+
map.put(Kind.FUNCTION_CALL, tree -> {
111+
FunctionCallTree functionCall = (FunctionCallTree) tree.getParent();
112+
return tree == functionCall.callee() || FUNCTION_ALLOWING_ARGUMENT_CHECK.contains(lowerCaseFunctionName(functionCall));
113+
});
114+
map.put(Kind.FOREACH_STATEMENT, tree -> tree == ((ForEachStatementTree) tree.getParent()).expression());
115+
map.put(Kind.ARRAY_ACCESS, tree -> !isArrayAssignment(tree));
116+
map.put(Kind.PARENTHESISED_EXPRESSION, tree -> isReadAccess(tree.getParent()));
117+
118+
return map;
119+
}
120+
121+
@Override
122+
public void visitFunctionDeclaration(FunctionDeclarationTree tree) {
123+
checkFunction(tree);
124+
super.visitFunctionDeclaration(tree);
125+
}
126+
127+
@Override
128+
public void visitFunctionExpression(FunctionExpressionTree tree) {
129+
checkFunction(tree);
130+
super.visitFunctionExpression(tree);
131+
}
132+
133+
@Override
134+
public void visitMethodDeclaration(MethodDeclarationTree tree) {
135+
checkFunction(tree);
136+
super.visitMethodDeclaration(tree);
137+
}
138+
139+
private void checkFunction(FunctionTree function) {
140+
VariableVisitor visitor = new VariableVisitor(function);
141+
function.accept(visitor);
142+
visitor.uninitializedStream()
143+
.forEach(variable -> context().newIssue(this, variable, "Review the data-flow - use of uninitialized value."));
144+
}
145+
146+
private static class VariableVisitor extends PHPVisitorCheck {
147+
148+
final FunctionTree currentFunction;
149+
150+
boolean trustVariables = true;
151+
152+
Map<String, VariableIdentifierTree> firstVariableReadAccess = new HashMap<>();
153+
154+
Set<String> initializedVariables = new HashSet<>();
155+
156+
VariableVisitor(FunctionTree currentFunction) {
157+
this.currentFunction = currentFunction;
158+
}
159+
160+
Stream<VariableIdentifierTree> uninitializedStream() {
161+
if (!trustVariables) {
162+
return Stream.empty();
163+
}
164+
return firstVariableReadAccess.entrySet().stream()
165+
.filter(entry -> !initializedVariables.contains(entry.getKey()))
166+
.map(Map.Entry::getValue);
167+
}
168+
169+
@Override
170+
public void visitVariableIdentifier(VariableIdentifierTree tree) {
171+
if (isClassMemberAccess(tree) || uninitializedVariableDeclaration(tree)) {
172+
return;
173+
}
174+
String name = tree.text();
175+
if (!PREDEFINED_VARIABLES.contains(name)) {
176+
if (isReadAccess(tree)) {
177+
if (!firstVariableReadAccess.containsKey(name)) {
178+
firstVariableReadAccess.put(name, tree);
179+
}
180+
} else {
181+
initializedVariables.add(name);
182+
}
183+
}
184+
}
185+
186+
@Override
187+
public void visitFunctionCall(FunctionCallTree functionCall) {
188+
if (FUNCTION_CHANGING_CURRENT_SCOPE.contains(lowerCaseFunctionName(functionCall))) {
189+
trustVariables = false;
190+
}
191+
super.visitFunctionCall(functionCall);
192+
}
193+
194+
@Override
195+
public void visitFunctionDeclaration(FunctionDeclarationTree tree) {
196+
if (tree == currentFunction) {
197+
super.visitFunctionDeclaration(tree);
198+
}
199+
// else skip nested
200+
}
201+
202+
@Override
203+
public void visitFunctionExpression(FunctionExpressionTree tree) {
204+
LexicalVariablesTree lexicalVars = tree.lexicalVars();
205+
if (tree == currentFunction) {
206+
if (lexicalVars != null) {
207+
lexicalVars.variables().stream()
208+
.map(VariableTree::variableExpression)
209+
.filter(variable -> variable.is(Kind.VARIABLE_IDENTIFIER))
210+
.map(variable -> ((VariableIdentifierTree) variable).text())
211+
.forEach(initializedVariables::add);
212+
}
213+
super.visitFunctionExpression(tree);
214+
} else {
215+
if (lexicalVars != null) {
216+
scan(lexicalVars.variables());
217+
}
218+
// skip nested
219+
}
220+
}
221+
222+
@Override
223+
public void visitClassDeclaration(ClassDeclarationTree tree) {
224+
// skip nested
225+
}
226+
227+
@Override
228+
public void visitAnonymousClass(AnonymousClassTree tree) {
229+
// skip nested
230+
}
231+
232+
private static boolean isClassMemberAccess(Tree tree) {
233+
Tree child = skipParentArrayAccess(tree);
234+
return (child.getParent().is(Kind.CLASS_MEMBER_ACCESS) &&
235+
((MemberAccessTree) child.getParent()).member() == child);
236+
}
237+
238+
private static boolean uninitializedVariableDeclaration(Tree tree) {
239+
return (tree.getParent().is(Kind.VARIABLE_DECLARATION) &&
240+
((VariableDeclarationTree) tree.getParent()).equalToken() == null);
241+
}
242+
243+
}
244+
245+
private static boolean isReadAccess(Tree tree) {
246+
Predicate<Tree> predicate = IS_READ_ACCESS_BY_PARENT_KIND.get(tree.getParent().getKind());
247+
return predicate == null || predicate.test(tree);
248+
}
249+
250+
@Nullable
251+
private static String lowerCaseFunctionName(FunctionCallTree functionCall) {
252+
String name = CheckUtils.getFunctionName(functionCall);
253+
return name != null ? name.toLowerCase(Locale.ROOT) : null;
254+
}
255+
256+
private static boolean isArrayAssignment(Tree tree) {
257+
Tree child = skipParentArrayAccess(tree);
258+
return child.getParent().is(Kind.ASSIGNMENT) &&
259+
((AssignmentExpressionTree) child.getParent()).variable() == child;
260+
}
261+
262+
private static Tree skipParentArrayAccess(Tree tree) {
263+
Tree child = tree;
264+
while (child.getParent().is(Kind.ARRAY_ACCESS) && ((ArrayAccessTree) child.getParent()).object() == child) {
265+
child = child.getParent();
266+
}
267+
return child;
268+
}
269+
270+
}

‎php-checks/src/main/resources/org/sonar/l10n/php/rules/php/S836.html

+28
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<p>When a variable is not initialized before its use, it's a sign that the developer made a mistake.</p>
2+
<h2>Noncompliant Code Example</h2>
3+
<pre>
4+
function fun($condition) {
5+
$res = 1;
6+
if ($condition) {
7+
$res++;
8+
}
9+
return $result; // Noncompliant, "$result" instead of "$res"
10+
}
11+
</pre>
12+
<h2>Compliant Solution</h2>
13+
<pre>
14+
function fun($condition) {
15+
$res = 1;
16+
if ($condition) {
17+
$res++;
18+
}
19+
return $res;
20+
}
21+
</pre>
22+
<h2>See</h2>
23+
<ul>
24+
<li> <a href="http://cwe.mitre.org/data/definitions/457.html">MITRE, CWE-457</a> - Use of Uninitialized Variable </li>
25+
<li> MISRA C:2004, 9.1 - All automatic variables shall have been assigned a value before being used. </li>
26+
<li> MISRA C++:2008, 8-5-1 - All variables shall have a defined value before they are used. </li>
27+
</ul>
28+

‎php-checks/src/main/resources/org/sonar/l10n/php/rules/php/S836.json

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
{
2+
"title": "Variables should be initialized before use",
3+
"type": "BUG",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "15min"
8+
},
9+
"tags": [
10+
"cwe",
11+
"misra"
12+
],
13+
"standards": [
14+
"CWE"
15+
],
16+
"defaultSeverity": "Major",
17+
"ruleSpecification": "RSPEC-836",
18+
"sqKey": "S836"
19+
}

‎php-checks/src/main/resources/org/sonar/l10n/php/rules/php/Sonar_way_profile.json

+1
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
"S127",
1414
"S131",
1515
"S138",
16+
"S836",
1617
"S905",
1718
"S907",
1819
"S1066",

‎php-checks/src/test/java/org/sonar/php/checks/UseOfUninitializedVariableCheckTest.java

+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/*
2+
* SonarQube PHP Plugin
3+
* Copyright (C) 2010-2017 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.php.checks;
21+
22+
import org.junit.Test;
23+
import org.sonar.plugins.php.CheckVerifier;
24+
25+
public class UseOfUninitializedVariableCheckTest {
26+
27+
@Test
28+
public void test() throws Exception {
29+
CheckVerifier.verify(new UseOfUninitializedVariableCheck(), "UseOfUninitializedVariableCheck.php");
30+
}
31+
32+
}

0 commit comments

Comments
 (0)