Triage some SpotBugs violations #6049

basil · 2021-12-11T21:16:47Z

This PR triages the following SpotBugs violations …

… and the following FindSecBugs violations …

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are correct
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

basil · 2021-12-11T21:17:20Z

core/src/main/java/hudson/ExtensionList.java

@@ -327,7 +328,7 @@ public T getDynamic(String className) {
     * Chooses the object that locks the loading of the extension instances.
     */
    protected Object getLoadLock() {
-        return jenkins.lookup.setIfNull(Lock.class,new Lock());
+        return Objects.requireNonNull(jenkins).lookup.setIfNull(Lock.class,new Lock());


Chases away NP_NULL_ON_SOME_PATH.

basil · 2021-12-11T21:17:25Z

core/src/main/java/hudson/ExtensionList.java

@@ -379,7 +380,7 @@ private void fireOnChangeListeners() {
            LOGGER.log(Level.FINER, String.format("Loading ExtensionList '%s' from", extensionType.getName()), new Throwable("Only present for stacktrace information"));
        }

-        return jenkins.getPluginManager().getPluginStrategy().findComponents(extensionType, hudson);
+        return Objects.requireNonNull(jenkins).getPluginManager().getPluginStrategy().findComponents(extensionType, hudson);


Chases away NP_NULL_ON_SOME_PATH.

basil · 2021-12-11T21:19:36Z

core/src/main/java/hudson/WebAppMain.java

-            // Tomcat breaks XSLT with JDK 5.0 and onward. Check if that's the case, and if so,
-            // try to correct it
-            try {
-                TransformerFactory.newInstance();
-                // if this works we are all happy
-            } catch (TransformerFactoryConfigurationError x) {
-                // no it didn't.
-                LOGGER.log(WARNING, "XSLT not configured correctly. Hudson will try to fix this. See https://bz.apache.org/bugzilla/show_bug.cgi?id=40895 for more details",x);
-                System.setProperty(TransformerFactory.class.getName(),"com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl");
-                try {
-                    TransformerFactory.newInstance();
-                    LOGGER.info("XSLT is set to the JAXP RI in JRE");
-                } catch(TransformerFactoryConfigurationError y) {
-                    LOGGER.log(SEVERE, "Failed to correct the problem.");
-                }
-            }
-


Was triggering XXE_DTD_TRANSFORM_FACTORY and XXE_XSLT_TRANSFORM_FACTORY, but the TransformerFactory is immediately thrown away rather than used for anything, so this is not actually a problem. Furthermore, this entire block of code is a workaround for a bug that was fixed in Tomcat 5.5, which was EOLed in 2012, so it's safe to say we don't need it anymore.

basil · 2021-12-11T21:20:38Z

core/src/main/java/hudson/model/AbstractItem.java

@@ -353,6 +353,7 @@ protected void checkRename(@NonNull String newName) throws Failure {
     * Not all the Items need to support this operation, but if you decide to do so,
     * you can use this method.
     */
+    @SuppressFBWarnings(value = "SWL_SLEEP_WITH_LOCK_HELD", justification = "no big deal")


The sleep is only done in an edge case (retrying when a file rename operation fails on Windows), so I think this is fine overall.

basil · 2021-12-11T21:20:53Z

core/src/main/java/hudson/model/AutoCompletionCandidates.java

@@ -108,6 +109,7 @@ public void generateResponse(StaplerRequest req, StaplerResponse rsp, Object o)
     *      The nearby contextual {@link ItemGroup} to resolve relative job names from.
     * @since 1.553
     */
+    @SuppressFBWarnings(value = "SBSC_USE_STRINGBUFFER_CONCATENATION", justification = "no big deal")


Premature optimization if I ever saw it.

basil · 2021-12-11T21:22:31Z

core/src/main/java/hudson/model/StringParameterDefinition.java

@@ -145,7 +145,7 @@ public String getHelpFile() {
    @Override
    public ParameterValue createValue(StaplerRequest req, JSONObject jo) {
        StringParameterValue value = req.bindJSON(StringParameterValue.class, jo);
-        if (isTrim() && value!=null) {
+        if (isTrim()) {


As of #5275

jenkins/core/src/main/java/hudson/model/StringParameterValue.java

Line 51 in 165d559

this.value = Util.fixNull(value);

means that this can no longer be null.

basil · 2021-12-11T21:23:34Z

core/src/main/java/hudson/security/ACL.java

@@ -355,7 +355,7 @@ public String toString() {
     */
    public static final Sid ANONYMOUS = new PrincipalSid(ANONYMOUS_USERNAME);

-    protected static final Sid[] AUTOMATIC_SIDS = new Sid[]{EVERYONE,ANONYMOUS};
+    static final Sid[] AUTOMATIC_SIDS = new Sid[]{EVERYONE,ANONYMOUS};


Triggers MS_PKGPROTECT because this is protected (as opposed to package-private) and mutable. SpotBugs has a point. Nothing else in the ecosystem is using it, so might as well restrict this for better hygiene and to keep SpotBugs happy.

basil · 2021-12-11T21:24:24Z

core/src/main/java/hudson/triggers/SCMTrigger.java

@@ -209,7 +210,7 @@ public DescriptorImpl getDescriptor() {
     * Returns the file that records the last/current polling activity.
     */
    public File getLogFile() {
-        return new File(job.getRootDir(),"scm-polling.log");
+        return new File(Objects.requireNonNull(job).getRootDir(),"scm-polling.log");


Chases away NP_NULL_ON_SOME_PATH.

basil · 2021-12-11T21:24:28Z

core/src/main/java/hudson/triggers/SCMTrigger.java

@@ -691,7 +692,7 @@ public boolean equals(Object that) {

        @Override
        public int hashCode() {
-            return job.hashCode();
+            return Objects.requireNonNull(job).hashCode();


Chases away NP_NULL_ON_SOME_PATH.

basil · 2021-12-11T21:25:18Z

core/src/main/java/jenkins/util/DirectedGraph.java

@@ -76,6 +77,7 @@ public int size() {
         */
        int lowlink;

+        @SuppressFBWarnings(value = "URF_UNREAD_FIELD", justification = "no big deal")


Technically unused, but useful when reading the code and following along in Tarjan's algorithm.

StefanSpieker · 2021-12-14T21:34:25Z

core/src/main/java/hudson/model/UpdateCenter.java

@@ -1240,6 +1240,7 @@ public void postValidate(DownloadJob job, File src) throws IOException {
         * @throws IOException if there were problems downloading the resource.
         * @see DownloadJob
         */
+        @SuppressFBWarnings(value = "WEAK_MESSAGE_DIGEST_SHA1", justification = "SHA-1 is only used as a fallback if SHA-256/SHA-512 are not available")


I think we might better drop SHA-1 totally.

Possibly, though I have no idea what constraints there are regarding compatibility and testing. Better left for a future PR I think.

StefanSpieker · 2021-12-14T21:36:30Z

core/src/main/java/jenkins/util/JSONSignatureValidator.java

@@ -52,6 +53,7 @@ public JSONSignatureValidator(String name) {
    /**
     * Verifies the signature in the update center data file.
     */
+    @SuppressFBWarnings(value = "WEAK_MESSAGE_DIGEST_SHA1", justification = "SHA-1 is only used as a fallback if SHA-512 is not available")


I think we might better drop SHA-1 totally.

core/src/main/java/hudson/ClassicPluginStrategy.java

core/src/main/java/hudson/node_monitors/ClockMonitor.java

core/src/main/java/hudson/util/ProcessTree.java

core/src/main/java/jenkins/fingerprints/FileFingerprintStorage.java

jglick · 2021-12-20T20:54:20Z

src/spotbugs/spotbugs-excludes.xml

        <Bug pattern="NP_NONNULL_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR"/>
        <Class name="jenkins.util.SystemProperties"/>


BTW I do not follow why entries like this were ever added to begin with. Simpler to just add @SuppressFBWarnings to the class in question with a justification?

Try removing these entries and adding @SuppressFBWarnings annotations for untriaged violations and see how long it takes you.

Well, I meant for the entries that have just one or two <Class>es.

I don't suppose it matters much either way, but I find it easier to triage stuff if the list of stuff to triage is in one place. I did add ~25 @SuppressFBWarnings(value = "[…]", justification = "TODO needs triage") annotations to various files where I couldn't easily suppress the warnings from spotbugs-excludes.xml (for example, because it was for an anonymous inner class that I couldn't easily refer to), but for the most part I added untriaged stuff to spotbugs-excludes.xml to keep the list in one place. I find it convenient to have it organized by violation type and class name in one file. Makes it easy to pick off a section and tackle it one area at a time.

…e` field too

… pending spotbugs/spotbugs#1515

basil · 2021-12-21T18:41:01Z

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!

Triage some SpotBugs violations

807dd56

basil added the skip-changelog Should not be shown in the changelog label Dec 11, 2021

basil commented Dec 11, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into spotbugs

5876d29

basil added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Dec 14, 2021

StefanSpieker reviewed Dec 14, 2021

View reviewed changes

StefanSpieker approved these changes Dec 15, 2021

View reviewed changes

basil added 2 commits December 15, 2021 10:31

Merge remote-tracking branch 'origin/master' into spotbugs

997d43c

Merge remote-tracking branch 'origin/master' into spotbugs

104eae0

basil requested a review from a team December 17, 2021 16:08

basil added 4 commits December 17, 2021 09:51

Merge remote-tracking branch 'origin/master' into spotbugs

daa98cf

Merge remote-tracking branch 'origin/master' into spotbugs

d728312

Add some Java 11-specific warnings to the triage list

cacec1c

Merge remote-tracking branch 'origin/master' into spotbugs

d0deb96

basil requested a review from a team December 20, 2021 17:06

jglick approved these changes Dec 20, 2021

View reviewed changes

basil added 3 commits December 20, 2021 13:01

Groovy will just ignore access modifiers and let you mutate a `privat…

db989a5

…e` field too

Move DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED to the exclude list…

90f812f

… pending spotbugs/spotbugs#1515

Merge remote-tracking branch 'origin/master' into spotbugs

b385eba

res0nance approved these changes Dec 21, 2021

View reviewed changes

basil added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 21, 2021

Merge remote-tracking branch 'origin/master' into spotbugs

8f073bd

basil merged commit a5f13e6 into jenkinsci:master Dec 22, 2021

basil deleted the spotbugs branch December 22, 2021 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Triage some SpotBugs violations #6049

Triage some SpotBugs violations #6049

basil commented Dec 11, 2021

basil Dec 11, 2021

basil Dec 11, 2021

basil Dec 11, 2021

basil Dec 11, 2021

basil Dec 11, 2021

basil Dec 11, 2021

basil Dec 11, 2021

basil Dec 11, 2021

basil Dec 11, 2021

basil Dec 11, 2021

StefanSpieker Dec 14, 2021

basil Dec 15, 2021

StefanSpieker Dec 14, 2021

jglick Dec 20, 2021

basil Dec 20, 2021

jglick Dec 20, 2021

basil Dec 20, 2021

basil commented Dec 21, 2021

		<Bug pattern="NP_NONNULL_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR"/>
		<Class name="jenkins.util.SystemProperties"/>

Triage some SpotBugs violations #6049

Triage some SpotBugs violations #6049

Conversation

basil commented Dec 11, 2021

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basil commented Dec 21, 2021