From b8aadc07de0e5c21ff428919ff12ebfc29c0960a Mon Sep 17 00:00:00 2001 From: Guillaume Toison <86775455+gtoison@users.noreply.github.com> Date: Wed, 29 Nov 2023 17:58:13 +0100 Subject: [PATCH] Upgrade SpotBugs to 4.8.2 (#880) --- .github/workflows/build.yml | 2 +- README.md | 4 +- generate_profiles/BuildXmlFiles.groovy | 4 +- pom.xml | 2 +- .../rules/FindbugsRulesDefinition.java | 4 +- .../profile-findbugs-and-fb-contrib.xml | 3 + .../findbugs/profile-findbugs-only.xml | 3 + .../sonar/plugins/findbugs/rules-findbugs.xml | 91 ++++++++++++------- 8 files changed, 70 insertions(+), 43 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 11ba1950..3232ba08 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -126,7 +126,7 @@ jobs: uses: ./.github/actions/sonar-update-center with: prop-file: findbugs.properties - description: Use SpotBugs 4.8.1, sb-contrib 7.6.0, and findsecbugs 1.12.0 + description: Use SpotBugs 4.8.2, sb-contrib 7.6.0, and findsecbugs 1.12.0 minimal-supported-sq-version: 9.9 latest-supported-sq-version: LATEST changelog-url: https://github.com/spotbugs/sonar-findbugs/releases/tag/${{ github.event.release.tag_name }} diff --git a/README.md b/README.md index 83ebf44e..1f78bc99 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # SonarQube Spotbugs Plugin [![.github/workflows/build.yml](https://github.com/spotbugs/sonar-findbugs/actions/workflows/build.yml/badge.svg)](https://github.com/spotbugs/sonar-findbugs/actions/workflows/build.yml) -![FindBugs Rules](https://img.shields.io/badge/SpotBugs_rules-909-brightgreen.svg?maxAge=2592000) +![FindBugs Rules](https://img.shields.io/badge/SpotBugs_rules-929-brightgreen.svg?maxAge=2592000) [![Coverage Status](https://sonarcloud.io/api/project_badges/measure?project=com.github.spotbugs%3Asonar-findbugs-plugin&metric=coverage)](https://sonarcloud.io/component_measures?id=com.github.spotbugs:sonar-findbugs-plugin&metric=coverage) ## Description / Features @@ -70,4 +70,4 @@ Findbugs Plugin version|Embedded SpotBugs/Findbugs version|Embedded Findsecbugs 4.2.3 | 4.7.3 (SpotBugs) | 1.12.0 | 7.4.7 (sb-contrib) | 1.8|7.9~|5.10.1.16922 4.2.4 | 4.7.3 (SpotBugs) | 1.12.0 | 7.6.0 (sb-contrib) | 1.8|7.9~|5.10.1.16922 4.2.5 | 4.8.1 (SpotBugs) | 1.12.0 | 7.6.0 (sb-contrib) | 1.8|7.9~|5.10.1.16922 -4.2.6-SNAPSHOT | 4.8.1 (SpotBugs) | 1.12.0 | 7.6.0 (sb-contrib) | 1.8|7.9~|5.10.1.16922 +4.2.6-SNAPSHOT | 4.8.2 (SpotBugs) | 1.12.0 | 7.6.0 (sb-contrib) | 1.8|7.9~|5.10.1.16922 diff --git a/generate_profiles/BuildXmlFiles.groovy b/generate_profiles/BuildXmlFiles.groovy index 287bd7b5..0523908e 100644 --- a/generate_profiles/BuildXmlFiles.groovy +++ b/generate_profiles/BuildXmlFiles.groovy @@ -8,13 +8,13 @@ import groovy.json.JsonSlurper; @Grapes([ - @Grab(group='com.github.spotbugs', module='spotbugs', version='4.8.1'), + @Grab(group='com.github.spotbugs', module='spotbugs', version='4.8.2'), @Grab(group='com.mebigfatguy.sb-contrib', module='sb-contrib', version='7.6.0'), @Grab(group='com.h3xstream.findsecbugs' , module='findsecbugs-plugin', version='1.12.0')] ) -FB = new Plugin(groupId: 'com.github.spotbugs', artifactId: 'spotbugs', version: '4.8.1') +FB = new Plugin(groupId: 'com.github.spotbugs', artifactId: 'spotbugs', version: '4.8.2') CONTRIB = new Plugin(groupId: 'com.mebigfatguy.sb-contrib', artifactId: 'sb-contrib', version: '7.6.0') FSB = new Plugin(groupId: 'com.h3xstream.findsecbugs', artifactId: 'findsecbugs-plugin', version: '1.12.0') diff --git a/pom.xml b/pom.xml index a69dce71..55b52bee 100644 --- a/pom.xml +++ b/pom.xml @@ -52,7 +52,7 @@ Also need to update profiles, see ./generate_profiles/README.md for detail. Update the version table and the rules count badge in README.md --> - 4.8.1 + 4.8.2 7.6.0 1.12.0 diff --git a/src/main/java/org/sonar/plugins/findbugs/rules/FindbugsRulesDefinition.java b/src/main/java/org/sonar/plugins/findbugs/rules/FindbugsRulesDefinition.java index fe2905bc..0ee96629 100644 --- a/src/main/java/org/sonar/plugins/findbugs/rules/FindbugsRulesDefinition.java +++ b/src/main/java/org/sonar/plugins/findbugs/rules/FindbugsRulesDefinition.java @@ -28,8 +28,8 @@ public final class FindbugsRulesDefinition implements RulesDefinition { public static final String REPOSITORY_KEY = "findbugs"; public static final String REPOSITORY_NAME = "FindBugs"; - public static final int RULE_COUNT = 475; - public static final int DEACTIVED_RULE_COUNT = 7; + public static final int RULE_COUNT = 476; + public static final int DEACTIVED_RULE_COUNT = 6; @Override public void define(Context context) { diff --git a/src/main/resources/org/sonar/plugins/findbugs/profile-findbugs-and-fb-contrib.xml b/src/main/resources/org/sonar/plugins/findbugs/profile-findbugs-and-fb-contrib.xml index bf8a51a8..3b7f7d3c 100644 --- a/src/main/resources/org/sonar/plugins/findbugs/profile-findbugs-and-fb-contrib.xml +++ b/src/main/resources/org/sonar/plugins/findbugs/profile-findbugs-and-fb-contrib.xml @@ -1424,6 +1424,9 @@ + + + diff --git a/src/main/resources/org/sonar/plugins/findbugs/profile-findbugs-only.xml b/src/main/resources/org/sonar/plugins/findbugs/profile-findbugs-only.xml index e7fc4160..34416335 100644 --- a/src/main/resources/org/sonar/plugins/findbugs/profile-findbugs-only.xml +++ b/src/main/resources/org/sonar/plugins/findbugs/profile-findbugs-only.xml @@ -1424,4 +1424,7 @@ + + + \ No newline at end of file diff --git a/src/main/resources/org/sonar/plugins/findbugs/rules-findbugs.xml b/src/main/resources/org/sonar/plugins/findbugs/rules-findbugs.xml index b4a306a5..fbb9c53e 100644 --- a/src/main/resources/org/sonar/plugins/findbugs/rules-findbugs.xml +++ b/src/main/resources/org/sonar/plugins/findbugs/rules-findbugs.xml @@ -119,7 +119,7 @@ While ScheduledThreadPoolExecutor inherits from ThreadPoolExecutor, a few of the Security - HTTP cookie formed from untrusted input HRS_REQUEST_PARAMETER_TO_COOKIE - <p>This code constructs an HTTP Cookie using an untrusted HTTP parameter. If this cookie is added to an HTTP response, it will allow a HTTP response splitting + <p>This code constructs an HTTP Cookie using an untrusted HTTP parameter. If this cookie is added to an HTTP response, it will allow an HTTP response splitting vulnerability. See <a href="http://en.wikipedia.org/wiki/HTTP_response_splitting">http://en.wikipedia.org/wiki/HTTP_response_splitting</a> for more information.</p> <p>SpotBugs looks only for the most blatant, obvious cases of HTTP response splitting. @@ -132,7 +132,7 @@ consider using a commercial static analysis or pen-testing tool. Security - HTTP Response splitting vulnerability HRS_REQUEST_PARAMETER_TO_HTTP_HEADER - <p>This code directly writes an HTTP parameter to an HTTP header, which allows for a HTTP response splitting + <p>This code directly writes an HTTP parameter to an HTTP header, which allows for an HTTP response splitting vulnerability. See <a href="http://en.wikipedia.org/wiki/HTTP_response_splitting">http://en.wikipedia.org/wiki/HTTP_response_splitting</a> for more information.</p> <p>SpotBugs looks only for the most blatant, obvious cases of HTTP response splitting. @@ -1473,7 +1473,7 @@ Thus, having a mutable instance field generally creates race conditions.<p> This code seems to be using non-short-circuit logic (e.g., &amp; or |) rather than short-circuit logic (&amp;&amp; or ||). In addition, -it seem possible that, depending on the value of the left hand side, you might not +it seems possible that, depending on the value of the left hand side, you might not want to evaluate the right hand side (because it would have side effects, could cause an exception or could be expensive.</p> <p> @@ -1526,7 +1526,7 @@ Language Specification</a> for details. will only give up one lock and the notify will be unable to get both locks, and thus the notify will not succeed. &nbsp; If there is also a warning about a two lock wait, the - probably of a bug is quite high. + probability of a bug is quite high. </p> <h2>Deprecated</h2> <p>This rule is deprecated</p> @@ -1934,7 +1934,7 @@ could be changed by malicious code or Style - Potentially ambiguous invocation of either an inherited or outer method IA_AMBIGUOUS_INVOCATION_OF_INHERITED_OR_OUTER_METHOD <p> -An inner class is invoking a method that could be resolved to either a inherited method or a method defined in an outer class. +An inner class is invoking a method that could be resolved to either an inherited method or a method defined in an outer class. For example, you invoke <code>foo(17)</code>, which is defined in both a superclass and in an outer method. By the Java semantics, it will be resolved to invoke the inherited method, but this may not be what @@ -2556,7 +2556,7 @@ dereferencing this value will generate a null pointer exception. UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR <p> This field is never initialized within any constructor, and is therefore could be null after the object is constructed. Elsewhere, it is loaded and dereferenced without a null check. -This could be a either an error or a questionable design, since +This could be either an error or a questionable design, since it means a null pointer exception will be generated if that field is dereferenced before being initialized. </p> @@ -2734,9 +2734,9 @@ is important or acceptable. Style - Return value of method without side effect is ignored RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT - <p>This code calls a method and ignores the return value. However our analysis shows that + <p>This code calls a method and ignores the return value. However, our analysis shows that the method (including its implementations in subclasses if any) does not produce any effect -other than return value. Thus this call can be removed. +other than return value. Thus, this call can be removed. </p> <p>We are trying to reduce the false positives as much as possible, but in some cases this warning might be wrong. Common false-positive cases include:</p> @@ -3235,7 +3235,7 @@ different types. The result of this comparison will always be false at runtime. <p> This method calls equals(Object) on two references of different class types and analysis suggests they will be to objects of different classes at runtime. Further, examination of the equals methods that would be invoked suggest that either -this call will always return false, or else the equals method is not be symmetric (which is +this call will always return false, or else the equals method is not symmetric (which is a property required by the contract for equals in class Object). </p> @@ -4055,7 +4055,7 @@ less confusing to explicitly check pointer equality using <code>==</cod Correctness - equals(...) used to compare incompatible arrays EC_INCOMPATIBLE_ARRAY_COMPARE <p> -This method invokes the .equals(Object o) to compare two arrays, but the arrays of +This method invokes the .equals(Object o) to compare two arrays, but the arrays of incompatible types (e.g., String[] and StringBuffer[], or String[] and int[]). They will never be equal. In addition, when equals(...) is used to compare arrays it only checks to see if they are the same array, and ignores the contents of the arrays. @@ -4429,7 +4429,7 @@ just use the constant. Methods detected are: reference). Client classes that use this class, may, in addition, use an instance of this class as a synchronizing object. Because two classes are using the same object for synchronization, Multithread correctness is suspect. You should not synchronize nor call semaphore methods on - a public reference. Consider using a internal private member variable to control synchronization. + a public reference. Consider using an internal private member variable to control synchronization. </p> style @@ -4709,7 +4709,7 @@ better to do a null test rather than an instanceof test. Style - Unchecked/unconfirmed cast BC_UNCONFIRMED_CAST <p> -This cast is unchecked, and not all instances of the type casted from can be cast to +This cast is unchecked, and not all instances of the type cast from can be cast to the type it is being cast to. Check that your program logic ensures that this cast will not fail. </p> @@ -5843,7 +5843,7 @@ Using floating-point variables should not be used as loop counters, as they are Bad practice - Assertion is used to validate an argument of a public method AA_ASSERTION_OF_ARGUMENTS - <p>Asssertions must not be used to validate arguments of public methods because the validations are + <p>Assertions must not be used to validate arguments of public methods because the validations are not performed if assertions are disabled.</p> <p> @@ -5856,7 +5856,7 @@ Using floating-point variables should not be used as loop counters, as they are Bad practice - Do not reuse public identifiers from JSL as class name PI_DO_NOT_REUSE_PUBLIC_IDENTIFIERS_CLASS_NAMES <p> - It's essential to avoid reusing public identifiers from the Java Standard Library as class names. + It's a good practice to avoid reusing public identifiers from the Java Standard Library as class names. This is because the Java Standard Library is a part of the Java platform and is expected to be available in all Java environments. Doing so can lead to naming conflicts and confusion, making it harder to understand and maintain the code. It's best practice to choose unique and descriptive class names that accurately represent the purpose and functionality of your own code. @@ -5925,7 +5925,7 @@ Using floating-point variables should not be used as loop counters, as they are Bad practice - Do not reuse public identifiers from JSL as method name PI_DO_NOT_REUSE_PUBLIC_IDENTIFIERS_LOCAL_VARIABLE_NAMES <p> - When declaring local variables in Java, it is crucial to refrain from reusing public identifiers from the Java Standard Library. + When declaring local variables in Java, it is a good practice to refrain from reusing public identifiers from the Java Standard Library. Reusing these identifiers as local variable names can lead to confusion, hinder code comprehension, and potentially cause conflicts with existing publicly available identifier names from the Java Standard Library. To maintain code clarity and avoid such issues, it is best practice to select unique and descriptive names for your local variables. @@ -5943,26 +5943,47 @@ Using floating-point variables should not be used as loop counters, as they are <p>See SEI CERT rule <a href="https://wiki.sei.cmu.edu/confluence/display/java/DCL01-J.+Do+not+reuse+public+identifiers+from+the+Java+Standard+Library">DCL01-J. Do not reuse public identifiers from the Java Standard Library</a>.</p> bad-practice - - Experimental - Do not reuse public identifiers from JSL as inner name - PI_DO_NOT_REUSE_PUBLIC_IDENTIFIERS_INNER_CLASS_NAMES - <p> - It's essential to avoid reusing public identifiers from the Java Standard Library as class names. - This is because the Java Standard Library is a part of the Java platform and is expected to be available in all Java environments. - Doing so can lead to naming conflicts and confusion, making it harder to understand and maintain the code. - It's best practice to choose unique and descriptive class names that accurately represent the purpose and functionality of your own code. - To provide an example, let's say you want to create an inner class for handling dates inside a specific Calendar class. Instead of using a common name like "Date", - which conflicts with the existing java.util.Date class, you could choose a more specific and unique name like or "CalendarDate" or "CalendarDisplayDate". - - A few key points to keep in mind when choosing names as identifier: - <ul> - <li>Use meaningful prefixes or namespaces: Prepend a project-specific prefix or namespace to your class names to make them distinct. For example, if your project is named "MyApp", you could use "MyAppDate" as your class name.</li> - <li>Use descriptive names: Opt for descriptive class names that clearly indicate their purpose and functionality. This helps avoid shadowing existing Java Standard Library identifiers. For instance, instead of "List", consider using "CustomAppList".</li> - <li>Follow naming conventions: Adhere to Java's naming conventions, such as using camel case (e.g., MyClass) for class names. This promotes code readability and reduces the chances of conflicts.</li> - </ul> - + + Bad practice - It is preferable to use portable Java property instead of environment variable. + ENV_USE_PROPERTY_INSTEAD_OF_ENV + <p> + Environment variables are not portable, the variable name itself (not only the value) may be different depending on the running OS. + Not only the names of the specific environment variables can differ (e.g. `USERNAME` in Windows and `USER` in Unix systems), + but even the semantics differ, e.g. the case sensitivity (Windows being case-insensitive and Unix case-sensitive). + Moreover, the Map of the environment variables returned by <code>java.lang.System.getenv()</code> and its collection views may not obey + the general contract of the <code>Object.equals(java.lang.Object)</code> and <code>Object.hashCode()</code> methods. + Consequently, using environment variables may have unintended side effects. + Also, the visibility of environment variables is less restricted compared to Java Properties: they are visible to all descendants + of the defining process, not just the immediate Java subprocess. + For these reasons, even the Java API of <code>java.lang.System</code> advises to use Java properties (<code>java.lang.System.getProperty(java.lang.String)</code>) + instead of environment variables (<code>java.lang.System.getenv(java.lang.String)</code>) where possible. </p> - <p>See SEI CERT rule <a href="https://wiki.sei.cmu.edu/confluence/display/java/DCL01-J.+Do+not+reuse+public+identifiers+from+the+Java+Standard+Library">DCL01-J. Do not reuse public identifiers from the Java Standard Library</a>.</p> - experimental + <p> + If a value can be accessed through both System.getProperty() and System.getenv(), it should be accessed using the former. + </p> + <p> + Mapping of corresponding Java System properties: + <table> + <tr> + <th>Environment variable</th> <th>Property</th> + </tr> + <tr><td>JAVA_HOME</td> <td>java.home</td></tr> + <tr><td>JAVA_VERSION</td> <td>java.version</td></tr> + <tr><td>TEMP</td> <td>java.io.tmpdir</td></tr> + <tr><td>TMP</td> <td>java.io.tmpdir</td></tr> + <tr><td>PROCESSOR_ARCHITECTURE</td> <td>os.arch</td></tr> + <tr><td>OS</td> <td>os.name</td></tr> + <tr><td>USER</td> <td>user.name</td></tr> + <tr><td>USERNAME</td> <td>user.name</td></tr> + <tr><td>HOME</td> <td>user.home</td></tr> + <tr><td>HOMEPATH</td> <td>user.home</td></tr> + <tr><td>CD</td> <td>user.dir</td></tr> + <tr><td>PWD</td> <td>user.dir</td></tr> + </table> + </p> + <p> + See SEI CERT rule <a href="https://wiki.sei.cmu.edu/confluence/display/java/ENV02-J.+Do+not+trust+the+values+of+environment+variables">ENV02-J. Do not trust the values of environment variables</a>. + </p> + bad-practice \ No newline at end of file