diff --git a/java-checks-test-sources/default/src/main/java/checks/PreparedStatementAndResultSetCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/PreparedStatementAndResultSetCheckSample.java index 23275de5ea0..16bcdf7acc9 100644 --- a/java-checks-test-sources/default/src/main/java/checks/PreparedStatementAndResultSetCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/PreparedStatementAndResultSetCheckSample.java @@ -155,10 +155,10 @@ public void updateCoffeeSales(HashMap salesForWeek, Connection updateString = "update " + param + ".COFFEES set SALES = ?"; PreparedStatement testParam = con.prepareStatement(updateString); - testParam.setInt(3, 0); // Noncompliant + testParam.setInt(3, 0); // Compliant - "param" is unknown and may itself contain placeholders testParam = con.prepareStatement(param + " update"); - testParam.setInt(3, 0); // Noncompliant + testParam.setInt(3, 0); // Compliant - "param" is unknown and may itself contain placeholders testParam = con.prepareStatement(param + param); testParam.setInt(3, 0); // Compliant @@ -257,4 +257,152 @@ void concatenation3(Connection con, String password, String name, int position) statement.setString(3, password); // compliant statement.setString(4, password); // Noncompliant } + + static class SqlConstants { + static final String SELECT = "SELECT * FROM t WHERE a = ?"; + static final String AND_CLAUSE = " AND b = ?"; + } + + private static final String COUNT_WRAPPER_BEGIN = "select count(*) from ("; + private static final String COUNT_WRAPPER_END = ")"; + + void unknownMethodCallInConcatenation(Connection con) throws SQLException { + String sql = getQuery(); + sql = COUNT_WRAPPER_BEGIN + sql + COUNT_WRAPPER_END; + + PreparedStatement ps = con.prepareStatement(sql); + ps.setInt(1, 1); // Compliant - part of the query is unknown + } + + void unknownMethodCallInPlusAssignment(Connection con) throws SQLException { + String sql = "select * from t where "; + sql += getQuery(); + + PreparedStatement ps = con.prepareStatement(sql); + ps.setInt(1, 1); // Compliant - part of the query is unknown + } + + void ternaryInPlusAssignment(Connection con, boolean isRecall) throws SQLException { + String sql = "SELECT * FROM t WHERE "; + sql += isRecall ? "LENDER_REF_ID = ? AND SENDER_ID = ?" : "BORROWER_REF_ID = ? AND RECIPIENT_ID = ?"; + + PreparedStatement ps = con.prepareStatement(sql); + ps.setString(1, "a"); // Compliant + ps.setLong(2, 1L); // Compliant + ps.setLong(3, 1L); // Noncompliant + } + + void ternaryWithDifferentBranches(Connection con, boolean withImage) throws SQLException { + String sql = "INSERT INTO Product (a, b" + (withImage ? ", c" : "") + ") VALUES (?, ?" + (withImage ? ", ?" : "") + ")"; + + PreparedStatement ps = con.prepareStatement(sql); + ps.setString(1, "a"); // Compliant + ps.setString(2, "b"); // Compliant + ps.setString(3, "c"); // Compliant - the third parameter exists when the condition holds + ps.setString(4, "d"); // Noncompliant + } + + void ternaryWithNonConstantFragment(Connection con, String tenant, int maxTurns) throws SQLException { + String sql = "SELECT * FROM m WHERE m.tenant = ? AND m.session_id = ? " + + (maxTurns > 0 ? "AND t.id IN (SELECT id FROM t WHERE tenant=? AND session=? LIMIT " + maxTurns + ") " : "") + + "ORDER BY m.seq"; + + PreparedStatement ps = con.prepareStatement(sql); + ps.setString(1, tenant); // Compliant + if (maxTurns > 0) { + ps.setString(3, tenant); // Compliant + ps.setString(4, tenant); // Compliant + } + // The whole query resolves: the int fragment contributes no placeholder and the branch taken contributes two. + ps.setString(5, tenant); // Noncompliant {{This "PreparedStatement" only has 4 parameters.}} + } + + void ternaryWithUnresolvableBranch(Connection con, boolean cond) throws SQLException { + PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE a = ? " + (cond ? getQuery() : "")); + ps.setString(5, "x"); // Compliant - one branch of the conditional is unknown, so the count is too + } + + void assignedInBothBranches(Connection con, boolean hasPassword) throws SQLException { + String sql; + if (hasPassword) { + sql = "UPDATE Account SET email = ?, name = ?, password = ? WHERE id = ?"; + } else { + sql = "UPDATE Account SET email = ?, name = ? WHERE id = ?"; + } + + PreparedStatement ps = con.prepareStatement(sql); + ps.setString(1, "a"); // Compliant + if (hasPassword) { + // FALSE POSITIVE: only the closest assignment is resolved, while the branch taken here has 4 parameters. + // Can't be fixed without dataflow analysis. + ps.setString(4, "b"); // Noncompliant + } + } + + void nonConstantNumericFragment(Connection con, int limit, Integer offset) throws SQLException { + String sql = "SELECT * FROM t WHERE id = ? LIMIT " + limit + " OFFSET " + offset; + + PreparedStatement ps = con.prepareStatement(sql); + ps.setInt(1, 1); // Compliant + // A number concatenated into the query can not add a placeholder, so the count stays exact. + ps.setInt(2, 2); // Noncompliant + } + + void nonConstantNumericFragmentFromMethod(Connection con) throws SQLException { + PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = ? LIMIT " + getIntValue()); + ps.setInt(2, 2); // Noncompliant + } + + void boxedCharFragment(Connection con, Character separator) throws SQLException { + PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = ? AND flag = " + separator); + ps.setInt(2, 2); // Compliant - the character may be '?' + ps.setInt(3, 2); // Noncompliant {{This "PreparedStatement" only has 2 parameters.}} + } + + void nonConstantCharFragment(Connection con, char separator) throws SQLException { + PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = ? AND flag = " + separator); + ps.setInt(2, 2); // Compliant - the character may be '?' + // A character adds at most one placeholder, so anything past that is still reportable. + ps.setInt(3, 2); // Noncompliant {{This "PreparedStatement" only has 2 parameters.}} + } + + private static final char QUESTION_MARK = '?'; + + void charConstantFragment(Connection con) throws SQLException { + PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = " + QUESTION_MARK); + // FALSE POSITIVE: index 1 is valid, the character constant is the placeholder. Constant folding renders a char as + // its numeric value, so the query resolves to "... WHERE id = 63" and the placeholder is lost. Fix belongs to the + // frontend, tracked by SONARJAVA-7228; remove this annotation once it lands. + ps.setInt(1, 1); // Noncompliant + } + + void charLiteralIsThePlaceholder(Connection con) throws SQLException { + PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = " + '?'); + ps.setInt(1, 1); // Compliant + ps.setInt(2, 2); // Noncompliant {{This "PreparedStatement" only has 1 parameters.}} + } + + void charLiteralIsNotAPlaceholder(Connection con) throws SQLException { + PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = ? AND flag = " + 'y'); + ps.setInt(1, 1); // Compliant + ps.setInt(2, 2); // Noncompliant {{This "PreparedStatement" only has 1 parameters.}} + } + + void charLocalVariableFragment(Connection con) throws SQLException { + // the char-typed fragment is bounded at one placeholder + final char questionMark = '?'; + PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = " + questionMark); + ps.setInt(1, 1); // Compliant + ps.setInt(2, 2); // Noncompliant {{This "PreparedStatement" only has 1 parameters.}} + } + + void constantsFromAnotherClass(Connection con) throws SQLException { + String sql = SqlConstants.SELECT; + sql += SqlConstants.AND_CLAUSE; + + PreparedStatement ps = con.prepareStatement(sql); + ps.setString(1, "a"); // Compliant + ps.setString(2, "b"); // Compliant + ps.setString(3, "c"); // Noncompliant + } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/PreparedStatementAndResultSetCheck.java b/java-checks/src/main/java/org/sonar/java/checks/PreparedStatementAndResultSetCheck.java index 22c1b6d0a8d..5e6dda996aa 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/PreparedStatementAndResultSetCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/PreparedStatementAndResultSetCheck.java @@ -26,9 +26,12 @@ import org.sonar.java.model.ExpressionUtils; import org.sonar.plugins.java.api.semantic.MethodMatchers; import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.semantic.Type.Primitives; import org.sonar.plugins.java.api.tree.Arguments; import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; import org.sonar.plugins.java.api.tree.BinaryExpressionTree; +import org.sonar.plugins.java.api.tree.ConditionalExpressionTree; import org.sonar.plugins.java.api.tree.ExpressionTree; import org.sonar.plugins.java.api.tree.IdentifierTree; import org.sonar.plugins.java.api.tree.LiteralTree; @@ -107,29 +110,45 @@ private static Integer getPreparedStatementNumberOfParameters(@Nullable Expressi } /** - * Count the number of query in the given expression. The method will reconstruct the string, if created in multiples steps. - * During the process, we are looking for closest reassignment of a variable. We will look for any reassignment before - * the first token of the argument "startingPointForReassignment". It is used to avoid infinite recursion in case the variable - * is reassigned and read in the same statement. + * Count the number of query parameters in the given expression. The method will reconstruct the string, if created in + * multiples steps. During the process, we are looking for closest reassignment of a variable. We will look for any + * reassignment before the first token of the argument "startingPointForReassignment". It is used to avoid infinite + * recursion in case the variable is reassigned and read in the same statement. + * Returns {@code null} as soon as any part of the query can not be resolved: an unresolved fragment may itself hold + * parameter placeholders, so the total count is unknown rather than zero. */ @CheckForNull private static Integer getNumberQuery(ExpressionTree expression, Tree startingPointForReassignment) { ExpressionTree expr = ExpressionUtils.skipParentheses(expression); + Integer unresolvedBound = unresolvedPlaceholderBound(expr); + if (unresolvedBound != null) { + return unresolvedBound; + } + Integer constantCount = countQuery(expr); + if (constantCount != null) { + return constantCount; + } if (expr.is(Tree.Kind.IDENTIFIER)) { return handleVariableUsedAsQuery((IdentifierTree) expr, startingPointForReassignment); - } else if (expr.is(Tree.Kind.PLUS)) { + } + if (expr.is(Tree.Kind.PLUS)) { return handleStringConcatenation((BinaryExpressionTree) expr, startingPointForReassignment); } - return countQuery(expr); + if (expr.is(Tree.Kind.CONDITIONAL_EXPRESSION)) { + return handleConditionalExpression((ConditionalExpressionTree) expr, startingPointForReassignment); + } + return null; } + @CheckForNull private static Integer handleVariableUsedAsQuery(IdentifierTree identifier, Tree startingPointForReassignment) { ExpressionTree lastAssignmentExpr = ReassignmentFinder.getClosestReassignmentOrDeclarationExpression(startingPointForReassignment, identifier.symbol()); if (lastAssignmentExpr != null) { Tree lastAssignment = lastAssignmentExpr.parent(); if (lastAssignment.is(Tree.Kind.PLUS_ASSIGNMENT)) { - return zeroIfNull(getNumberQuery(lastAssignmentExpr, lastAssignment)) - + zeroIfNull(getNumberQuery(((AssignmentExpressionTree) lastAssignment).variable(), lastAssignment)); + return sumOrNull( + getNumberQuery(((AssignmentExpressionTree) lastAssignment).variable(), lastAssignment), + getNumberQuery(lastAssignmentExpr, lastAssignment)); } // If the current assignment contains the identifier, we have to update the starting point for reassignment to avoid infinite recursion. return getNumberQuery(lastAssignmentExpr, lastAssignment); @@ -137,18 +156,59 @@ private static Integer handleVariableUsedAsQuery(IdentifierTree identifier, Tree return null; } + @CheckForNull private static Integer handleStringConcatenation(BinaryExpressionTree expr, Tree startingPointForReassignment) { - Integer left = getNumberQuery(expr.leftOperand(), startingPointForReassignment); - Integer right = getNumberQuery(expr.rightOperand(), startingPointForReassignment); - return (left == null && right == null) ? null : (zeroIfNull(left) + zeroIfNull(right)); + return sumOrNull( + getNumberQuery(expr.leftOperand(), startingPointForReassignment), + getNumberQuery(expr.rightOperand(), startingPointForReassignment)); + } + + /** + * A conditional expression contributes as many placeholders as its most parameterized branch: reporting against the + * smaller branch would flag indices which are valid whenever the other branch is taken. + */ + @CheckForNull + private static Integer handleConditionalExpression(ConditionalExpressionTree expr, Tree startingPointForReassignment) { + Integer trueBranch = getNumberQuery(expr.trueExpression(), startingPointForReassignment); + Integer falseBranch = getNumberQuery(expr.falseExpression(), startingPointForReassignment); + if (trueBranch == null || falseBranch == null) { + return null; + } + return Math.max(trueBranch, falseBranch); + } + + /** + * Upper bound on the placeholders an operand can contribute when its value is unknown: none for a number or a + * boolean, whatever their runtime value, and at most one for a character, since '?' is a valid one. Returns + * {@code null} for anything that can hold an arbitrary number of them. + */ + @CheckForNull + private static Integer unresolvedPlaceholderBound(ExpressionTree expression) { + if (expression.is(Tree.Kind.CHAR_LITERAL)) { + return "'?'".equals(((LiteralTree) expression).value()) ? 1 : 0; + } + Type type = expression.symbolType(); + if (type.isPrimitive(Primitives.CHAR) || type.is("java.lang.Character")) { + return 1; + } + if (type.isPrimitive() || type.isPrimitiveWrapper()) { + return 0; + } + return null; } - private static int zeroIfNull(@Nullable Integer intValue) { - return intValue == null ? 0 : intValue; + @CheckForNull + private static Integer sumOrNull(@Nullable Integer left, @Nullable Integer right) { + if (left == null || right == null) { + return null; + } + return left + right; } @CheckForNull private static Integer countQuery(ExpressionTree expression) { - return expression.is(Tree.Kind.STRING_LITERAL) ? StringUtils.countMatches(((LiteralTree) expression).value(), "?") : null; + return expression.asConstant(String.class) + .map(query -> StringUtils.countMatches(query, "?")) + .orElse(null); } }