From 00bd670861d465a52e24a607e249383dd267a155 Mon Sep 17 00:00:00 2001 From: Judit Knoll <123470644+JuditKnoll@users.noreply.github.com> Date: Thu, 9 Nov 2023 02:05:05 +0100 Subject: [PATCH 1/4] Fix small grammar mistakes in messages.xml (#2690) --- spotbugs/etc/messages.xml | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/spotbugs/etc/messages.xml b/spotbugs/etc/messages.xml index 3d0d051e374..4047e1d90a6 100644 --- a/spotbugs/etc/messages.xml +++ b/spotbugs/etc/messages.xml @@ -2027,7 +2027,7 @@ While ScheduledThreadPoolExecutor inherits from ThreadPoolExecutor, a few of the HTTP cookie formed from untrusted input in {1}
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 +

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 http://en.wikipedia.org/wiki/HTTP_response_splitting for more information.

SpotBugs looks only for the most blatant, obvious cases of HTTP response splitting. @@ -2044,7 +2044,7 @@ consider using a commercial static analysis or pen-testing tool. HTTP parameter directly written to HTTP header output in {1}

This code directly writes an HTTP parameter to an HTTP header, which allows for a HTTP response splitting +

This code directly writes an HTTP parameter to an HTTP header, which allows for an HTTP response splitting vulnerability. See http://en.wikipedia.org/wiki/HTTP_response_splitting for more information.

SpotBugs looks only for the most blatant, obvious cases of HTTP response splitting. @@ -3770,7 +3770,7 @@ Thus, having a mutable instance field generally creates race conditions.

This code seems to be using non-short-circuit logic (e.g., & or |) rather than short-circuit logic (&& 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.

@@ -3831,7 +3831,7 @@ Language Specification 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.   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.

]]>
@@ -4309,7 +4309,7 @@ could be changed by malicious code or
-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 foo(17), 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 @@ -5090,7 +5090,7 @@ dereferencing this value will generate a null pointer exception. 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.

@@ -5315,9 +5315,9 @@ is important or acceptable. Return value of {2.givenClass} ignored, but method has no side effect
This code calls a method and ignores the return value. However our analysis shows that +

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.

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:

@@ -5918,7 +5918,7 @@ different types. The result of this comparison will always be false at runtime.

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).

@@ -6881,7 +6881,7 @@ less confusing to explicitly check pointer equality using ==.
-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. @@ -7317,7 +7317,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.

]]>
@@ -7645,7 +7645,7 @@ better to do a null test rather than an instanceof test.
-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.

@@ -8995,7 +8995,7 @@ Using floating-point variables should not be used as loop counters, as they are Assertion validates method argument at {1}. If assertions are disabled, there won't be any argument validation.
Asssertions must not be used to validate arguments of public methods because the validations are +

Assertions must not be used to validate arguments of public methods because the validations are not performed if assertions are disabled.

From 95f17542e43dc56cccfdfc7e82ac64e9ac95a55a Mon Sep 17 00:00:00 2001 From: Guillaume Toison <86775455+gtoison@users.noreply.github.com> Date: Sat, 11 Nov 2023 02:05:54 +0100 Subject: [PATCH 2/4] Use java.nio to load filter files (#2684) * test: load a filter with non-ascii characters in file name * Use java.nio to load filter files java.io.FileInputStream seems to be running into issues when trying to load files with non-ascii charatecter in the file name. Other similar issues point to this happening on a Mac, so it might be dependent on the combination of OS/JDK version/user settings Originally reported here: https://github.com/JetBrains/spotbugs-intellij-plugin/issues/1492 --- CHANGELOG.md | 1 + .../filter/Utf8FilterFileNameTest.java | 52 +++++++++++++++++++ .../edu/umd/cs/findbugs/filter/Filter.java | 7 ++- 3 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 spotbugs-tests/src/test/java/edu/umd/cs/findbugs/filter/Utf8FilterFileNameTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c9dc898d7e..c53b1a6314c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2. ### Fixed - Fixed false positive UPM_UNCALLED_PRIVATE_METHOD for method used in JUnit's MethodSource ([[#2379](https://github.com/spotbugs/spotbugs/issues/2379)]) +- Use java.nio to load filter files ([[#2684](https://github.com/spotbugs/spotbugs/pull/2684)]) - tbd diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/filter/Utf8FilterFileNameTest.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/filter/Utf8FilterFileNameTest.java new file mode 100644 index 00000000000..544b09ef2b9 --- /dev/null +++ b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/filter/Utf8FilterFileNameTest.java @@ -0,0 +1,52 @@ +/* + * Contributions to SpotBugs + * Copyright (C) 2023, the SpotBugs authors + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ +package edu.umd.cs.findbugs.filter; + +import static org.junit.jupiter.api.Assertions.fail; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; + + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + + +/** + * @author gtoison + */ +class Utf8FilterFileNameTest { + @TempDir + private Path folderPath; + + @Test + void loadFilter() { + Path filterPath = folderPath.resolve("äéàùçæð.xml"); + + try { + Files.createFile(filterPath); + Files.writeString(filterPath, ""); + + Filter filter = new Filter(filterPath.toAbsolutePath().toString()); + } catch (IOException e) { + fail("Error loading filter file " + filterPath, e); + } + } +} diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/filter/Filter.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/filter/Filter.java index e90ef88b5bc..8c969831a21 100644 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/filter/Filter.java +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/filter/Filter.java @@ -20,11 +20,13 @@ package edu.umd.cs.findbugs.filter; import java.io.File; -import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.io.Reader; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.IdentityHashMap; import java.util.Iterator; @@ -204,7 +206,8 @@ public boolean match(BugInstance bugInstance) { * @throws ParserConfigurationException */ private void parse(String fileName) throws IOException, SAXException, ParserConfigurationException { - FileInputStream fileInputStream = new FileInputStream(new File(fileName)); + Path path = FileSystems.getDefault().getPath(fileName); + InputStream fileInputStream = Files.newInputStream(path); parse(fileName, fileInputStream); } From f839cacf853db09e895b761ccaa35788ae2173ea Mon Sep 17 00:00:00 2001 From: Andrey Loskutov Date: Tue, 14 Nov 2023 19:40:16 +0100 Subject: [PATCH 3/4] Do not export "javax.annotation" packages This prevents spotbugs provided "javax.annotation" packages to be consumed by bundles that expect default (not spotbugs) "javax.annotation" packages that were previously shipped with "javax.annotation" bundle with SDK. The problem appears since 4.30 SDK doesn't ship "javax.annotation" bundle anymore, so spotbugs exported packages are taken as alternative if the application doesn't have other providers and in worst case they are not what applications actually expected to receive or cause dependency cycles (spotbugs requires some bundle that requires some other that imports "javax.annotation" package that is coming from spotbugs). The packages originally were needed by detectors shipped as bundles for IDE, but this is a very seldom case and in that case spotbugs-annotations is the better alternative. We should not export "wrong" javax packages anymore, it wasn't nice and it can cause only trouble now. See https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/issues/1056 --- CHANGELOG.md | 1 + eclipsePlugin/META-INF/MANIFEST-TEMPLATE.MF | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c53b1a6314c..d2a47e27f4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2. ### Fixed - Fixed false positive UPM_UNCALLED_PRIVATE_METHOD for method used in JUnit's MethodSource ([[#2379](https://github.com/spotbugs/spotbugs/issues/2379)]) - Use java.nio to load filter files ([[#2684](https://github.com/spotbugs/spotbugs/pull/2684)]) +- Eclipse: Do not export javax.annotation packages ([[#2699](https://github.com/spotbugs/spotbugs/pull/2699)]) - tbd diff --git a/eclipsePlugin/META-INF/MANIFEST-TEMPLATE.MF b/eclipsePlugin/META-INF/MANIFEST-TEMPLATE.MF index 0bba919740c..85a1a09e120 100644 --- a/eclipsePlugin/META-INF/MANIFEST-TEMPLATE.MF +++ b/eclipsePlugin/META-INF/MANIFEST-TEMPLATE.MF @@ -97,9 +97,6 @@ Export-Package: de.tobject.findbugs, edu.umd.cs.findbugs.visitclass, edu.umd.cs.findbugs.workflow, edu.umd.cs.findbugs.xml, - javax.annotation, - javax.annotation.concurrent, - javax.annotation.meta, org.apache.bcel, org.apache.bcel.classfile, org.apache.bcel.generic, From a9ed9ef169fe4274962813f9d3a7d5e662dcc128 Mon Sep 17 00:00:00 2001 From: Andrey Loskutov Date: Wed, 15 Nov 2023 11:49:19 +0100 Subject: [PATCH 4/4] FindOverridableMethodCall is not thread safe Don't use static maps for analysis specific data, it can't work with multiple analysis running in same JVM, like it might happen in Eclipse. Fixes https://github.com/spotbugs/spotbugs/issues/2701 --- CHANGELOG.md | 1 + .../detect/FindOverridableMethodCall.java | 20 +++++++++---------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d2a47e27f4c..23c3996e6f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2. - Fixed false positive UPM_UNCALLED_PRIVATE_METHOD for method used in JUnit's MethodSource ([[#2379](https://github.com/spotbugs/spotbugs/issues/2379)]) - Use java.nio to load filter files ([[#2684](https://github.com/spotbugs/spotbugs/pull/2684)]) - Eclipse: Do not export javax.annotation packages ([[#2699](https://github.com/spotbugs/spotbugs/pull/2699)]) +- Fixed not thread safe FindOverridableMethodCall detector ([[#2701](https://github.com/spotbugs/spotbugs/issues/2701)]) - tbd diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindOverridableMethodCall.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindOverridableMethodCall.java index 9229111950b..69bb9fd4ac2 100644 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindOverridableMethodCall.java +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindOverridableMethodCall.java @@ -59,16 +59,16 @@ private static class CallerInfo { } // For methods called using the standard way - private static final Map callerConstructors = new HashMap<>(); - private static final Map callerClones = new HashMap<>(); - private static final Map callsToOverridable = new HashMap<>(); - private static final MultiMap callerToCalleeMap = new MultiMap<>(ArrayList.class); - private static final MultiMap calleeToCallerMap = new MultiMap<>(ArrayList.class); + private final Map callerConstructors = new HashMap<>(); + private final Map callerClones = new HashMap<>(); + private final Map callsToOverridable = new HashMap<>(); + private final MultiMap callerToCalleeMap = new MultiMap<>(ArrayList.class); + private final MultiMap calleeToCallerMap = new MultiMap<>(ArrayList.class); // For methods called using method references - private static final Map refCallerConstructors = new HashMap<>(); - private static final Map refCallerClones = new HashMap<>(); - private static final MultiMap refCalleeToCallerMap = new MultiMap<>(ArrayList.class); + private final Map refCallerConstructors = new HashMap<>(); + private final Map refCallerClones = new HashMap<>(); + private final MultiMap refCalleeToCallerMap = new MultiMap<>(ArrayList.class); private final BugAccumulator bugAccumulator; @@ -303,7 +303,7 @@ private boolean checkAndRecordCallBetweenNonOverridableMethods(XMethod caller, X } private XMethod getIndirectlyCalledOverridable(XMethod caller) { - return getIndirectlyCalledOverridable(caller, new HashSet()); + return getIndirectlyCalledOverridable(caller, new HashSet<>()); } private XMethod getIndirectlyCalledOverridable(XMethod caller, Set visited) { @@ -334,7 +334,7 @@ private CallerInfo getIndirectCallerClone(XMethod callee) { } private CallerInfo getIndirectCallerSpecial(XMethod callee, Map map) { - return getIndirectCallerSpecial(callee, map, new HashSet()); + return getIndirectCallerSpecial(callee, map, new HashSet<>()); } private CallerInfo getIndirectCallerSpecial(XMethod callee, Map map, Set visited) {