diff --git a/java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected b/java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected index 6f396573aa16..2cff4a3eaa62 100644 --- a/java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected +++ b/java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected @@ -9,3 +9,4 @@ ql/java/ql/src/Likely Bugs/Likely Typos/ContradictoryTypeChecks.ql ql/java/ql/src/Likely Bugs/Likely Typos/SuspiciousDateFormat.ql ql/java/ql/src/Likely Bugs/Resource Leaks/CloseReader.ql ql/java/ql/src/Likely Bugs/Resource Leaks/CloseWriter.ql +ql/java/ql/src/Performance/StringReplaceAllWithNonRegex.ql diff --git a/java/ql/src/Performance/StringReplaceAllWithNonRegex.md b/java/ql/src/Performance/StringReplaceAllWithNonRegex.md new file mode 100644 index 000000000000..6e298b4955b6 --- /dev/null +++ b/java/ql/src/Performance/StringReplaceAllWithNonRegex.md @@ -0,0 +1,29 @@ +# Use of `String#replaceAll` with a first argument which is not a regular expression + +Using `String#replaceAll` is less performant than `String#replace` when the first argument is not a regular expression. + +## Overview + +The `String#replaceAll` method is designed to work with regular expressions as its first parameter. When you use a simple string without any regex patterns (like special characters or syntax), it's more efficient to use `String#replace` instead. This is because `replaceAll` has to compile the input as a regular expression first, which adds unnecessary overhead when you are just replacing literal text. + +## Recommendation + +Use `String#replace` instead where a `replaceAll` call uses a trivial string as its first argument. + +## Example + +```java +public class Test { + void f() { + String s1 = "test"; + s1 = s1.replaceAll("t", "x"); // NON_COMPLIANT + s1 = s1.replaceAll(".*", "x"); // COMPLIANT + } +} + +``` + +## References + +- Java SE Documentation: [String.replaceAll](https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/lang/String.html#replaceAll(java.lang.String,java.lang.String)). +- Common Weakness Enumeration: [CWE-1176](https://cwe.mitre.org/data/definitions/1176.html). diff --git a/java/ql/src/Performance/StringReplaceAllWithNonRegex.ql b/java/ql/src/Performance/StringReplaceAllWithNonRegex.ql new file mode 100644 index 000000000000..bc05b3bf063b --- /dev/null +++ b/java/ql/src/Performance/StringReplaceAllWithNonRegex.ql @@ -0,0 +1,24 @@ +/** + * @id java/string-replace-all-with-non-regex + * @name Use of `String#replaceAll` with a first argument which is not a regular expression + * @description Using `String#replaceAll` with a first argument which is not a regular expression + * is less efficient than using `String#replace`. + * @kind problem + * @precision very-high + * @problem.severity recommendation + * @tags quality + * reliability + * performance + * external/cwe/cwe-1176 + */ + +import java + +from StringReplaceAllCall replaceAllCall, StringLiteral firstArg +where + firstArg = replaceAllCall.getArgument(0) and + //only contains characters that could be a simple string + firstArg.getValue().regexpMatch("^[a-zA-Z0-9]+$") +select replaceAllCall, + "This call to 'replaceAll' should be a call to 'replace' as its $@ is not a regular expression.", + firstArg, "first argument" diff --git a/java/ql/src/codeql-suites/java-code-quality.qls b/java/ql/src/codeql-suites/java-code-quality.qls index ac1f52624c4f..2eafe785532e 100644 --- a/java/ql/src/codeql-suites/java-code-quality.qls +++ b/java/ql/src/codeql-suites/java-code-quality.qls @@ -1,14 +1,15 @@ - queries: . - include: id: - - java/suspicious-date-format - - java/integer-multiplication-cast-to-long - - java/equals-on-unrelated-types - java/contradictory-type-checks - - java/reference-equality-of-boxed-types + - java/equals-on-unrelated-types - java/inconsistent-equals-and-hashcode - - java/unchecked-cast-in-equals - - java/unused-container - java/input-resource-leak + - java/integer-multiplication-cast-to-long - java/output-resource-leak - - java/type-variable-hides-type \ No newline at end of file + - java/reference-equality-of-boxed-types + - java/string-replace-all-with-non-regex + - java/suspicious-date-format + - java/type-variable-hides-type + - java/unchecked-cast-in-equals + - java/unused-container diff --git a/java/ql/test/query-tests/StringReplaceAllWithNonRegex/StringReplaceAllWithNonRegex.expected b/java/ql/test/query-tests/StringReplaceAllWithNonRegex/StringReplaceAllWithNonRegex.expected new file mode 100644 index 000000000000..944dd3d23a3d --- /dev/null +++ b/java/ql/test/query-tests/StringReplaceAllWithNonRegex/StringReplaceAllWithNonRegex.expected @@ -0,0 +1 @@ +| Test.java:4:14:4:36 | replaceAll(...) | This call to 'replaceAll' should be a call to 'replace' as its $@ is not a regular expression. | Test.java:4:28:4:30 | "t" | first argument | diff --git a/java/ql/test/query-tests/StringReplaceAllWithNonRegex/StringReplaceAllWithNonRegex.qlref b/java/ql/test/query-tests/StringReplaceAllWithNonRegex/StringReplaceAllWithNonRegex.qlref new file mode 100644 index 000000000000..7737507b19e9 --- /dev/null +++ b/java/ql/test/query-tests/StringReplaceAllWithNonRegex/StringReplaceAllWithNonRegex.qlref @@ -0,0 +1,2 @@ +query: Performance/StringReplaceAllWithNonRegex.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql diff --git a/java/ql/test/query-tests/StringReplaceAllWithNonRegex/Test.java b/java/ql/test/query-tests/StringReplaceAllWithNonRegex/Test.java new file mode 100644 index 000000000000..1465343b8c2e --- /dev/null +++ b/java/ql/test/query-tests/StringReplaceAllWithNonRegex/Test.java @@ -0,0 +1,7 @@ +public class Test { + void f() { + String s1 = "test"; + s1 = s1.replaceAll("t", "x"); // $ Alert // NON_COMPLIANT + s1 = s1.replaceAll(".*", "x"); // COMPLIANT + } +}