[JENKINS-29790] Noting merge of #1783.

jenkinsci · Aug 24, 2015 · 434608b · 434608b
2 parents 5e9492d + c26fd23
commit 434608b
Show file tree

Hide file tree

Showing 12 changed files with 142 additions and 25 deletions.
diff --git a/changelog.html b/changelog.html
@@ -55,7 +55,9 @@
 <!-- Record your changes in the trunk here. -->
 <div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
 <ul class=image>
-  <li class=>
+  <li class="major bug">
+    Race condition in triggers could cause various <code>NullPointerException</code>s.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-29790">issue 29790</a>)
 </ul>
 </div><!--=TRUNK-END=-->
 <h3><a name=v1.626>What's new in 1.626</a> (2015/08/23)</h3>

diff --git a/core/src/main/java/hudson/scheduler/CronTabList.java b/core/src/main/java/hudson/scheduler/CronTabList.java
@@ -29,6 +29,8 @@
 import java.util.Collection;
 import java.util.Vector;
 import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
+
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
@@ -44,7 +46,7 @@ public final class CronTabList {
     private final Vector<CronTab> tabs;
 
     public CronTabList(Collection<CronTab> tabs) {
-        this.tabs = new Vector<CronTab>(tabs);
+        this.tabs = new Vector<>(tabs);
     }
 
     /**
@@ -90,12 +92,12 @@ public String checkSanity() {
         return null;
     }
 
-    public static CronTabList create(String format) throws ANTLRException {
+    public static CronTabList create(@Nonnull String format) throws ANTLRException {
         return create(format,null);
     }
 
-    public static CronTabList create(String format, Hash hash) throws ANTLRException {
-        Vector<CronTab> r = new Vector<CronTab>();
+    public static CronTabList create(@Nonnull String format, Hash hash) throws ANTLRException {
+        Vector<CronTab> r = new Vector<>();
         int lineNumber = 0;
         String timezone = null;
 

diff --git a/core/src/main/java/hudson/triggers/SCMTrigger.java b/core/src/main/java/hudson/triggers/SCMTrigger.java
@@ -2,6 +2,7 @@
  * The MIT License
  * 
  * Copyright (c) 2004-2009, Sun Microsystems, Inc., Kohsuke Kawaguchi, Brian Westrich, Jean-Baptiste Quenot, id:cactusman
+ *               2015 Kanstantsin Shautsou
  * 
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -24,6 +25,7 @@
 package hudson.triggers;
 
 import antlr.ANTLRException;
+import com.google.common.base.Preconditions;
 import hudson.Extension;
 import hudson.Util;
 import hudson.console.AnnotatedLargeText;
@@ -117,6 +119,10 @@ public boolean isIgnorePostCommitHooks() {
 
     @Override
     public void run() {
+        if (job == null) {
+            return;
+        }
+
         run(null);
     }
 
@@ -128,6 +134,10 @@ public void run() {
      * @since 1.375
      */
     public void run(Action[] additionalActions) {
+        if (job == null) {
+            return;
+        }
+
         DescriptorImpl d = getDescriptor();
 
         LOGGER.fine("Scheduling a polling for "+job);
@@ -152,6 +162,10 @@ public DescriptorImpl getDescriptor() {
 
     @Override
     public Collection<? extends Action> getProjectActions() {
+        if (job == null) {
+            return Collections.emptyList();
+        }
+
         return Collections.singleton(new SCMAction());
     }
 
@@ -457,10 +471,12 @@ public class Runner implements Runnable {
         private Action[] additionalActions;
 
         public Runner() {
-            additionalActions = new Action[0];
+            this(null);
         }
 
         public Runner(Action[] actions) {
+            Preconditions.checkNotNull(job, "Runner can't be instantiated when job is null");
+
             if (actions == null) {
                 additionalActions = new Action[0];
             } else {
@@ -514,11 +530,7 @@ private boolean runPolling() {
                     else
                         logger.println("No changes");
                     return result;
-                } catch (Error e) {
-                    e.printStackTrace(listener.error("Failed to record SCM polling for "+job));
-                    LOGGER.log(Level.SEVERE,"Failed to record SCM polling for "+job,e);
-                    throw e;
-                } catch (RuntimeException e) {
+                } catch (Error | RuntimeException e) {
                     e.printStackTrace(listener.error("Failed to record SCM polling for "+job));
                     LOGGER.log(Level.SEVERE,"Failed to record SCM polling for "+job,e);
                     throw e;
@@ -532,6 +544,10 @@ private boolean runPolling() {
         }
 
         public void run() {
+            if (job == null) {
+                return;
+            }
+
             String threadName = Thread.currentThread().getName();
             Thread.currentThread().setName("SCM polling for "+job);
             try {

diff --git a/core/src/main/java/hudson/triggers/TimerTrigger.java b/core/src/main/java/hudson/triggers/TimerTrigger.java
@@ -2,6 +2,7 @@
  * The MIT License
  * 
  * Copyright (c) 2004-2009, Sun Microsystems, Inc., Kohsuke Kawaguchi, Jean-Baptiste Quenot, Martin Eigenbrodt
+ *               2015 Kanstantsin Shautsou
  * 
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -38,6 +39,8 @@
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.QueryParameter;
 
+import javax.annotation.Nonnull;
+
 /**
  * {@link Trigger} that runs a job periodically.
  *
@@ -46,12 +49,16 @@
 public class TimerTrigger extends Trigger<BuildableItem> {
 
     @DataBoundConstructor
-    public TimerTrigger(String spec) throws ANTLRException {
+    public TimerTrigger(@Nonnull String spec) throws ANTLRException {
         super(spec);
     }
 
     @Override
     public void run() {
+        if (job == null) {
+            return;
+        }
+
         job.scheduleBuild(0, new TimerTriggerCause());
     }
 

diff --git a/core/src/main/java/hudson/triggers/Trigger.java b/core/src/main/java/hudson/triggers/Trigger.java
@@ -2,7 +2,8 @@
  * The MIT License
  * 
  * Copyright (c) 2004-2009, Sun Microsystems, Inc., Kohsuke Kawaguchi, Brian Westrich, Jean-Baptiste Quenot, Stephen Connolly, Tom Huybrechts
- * 
+ *               2015 Kanstantsin Shautsou
+ *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
  * in the Software without restriction, including without limitation the rights
@@ -57,6 +58,8 @@
 
 import antlr.ANTLRException;
 import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
+
 import edu.umd.cs.findbugs.annotations.SuppressWarnings;
 import hudson.model.Items;
 import jenkins.model.ParameterizedJobMixIn;
@@ -149,14 +152,15 @@ public TriggerDescriptor getDescriptor() {
 
     protected final String spec;
     protected transient CronTabList tabs;
+    @CheckForNull
     protected transient J job;
 
     /**
      * Creates a new {@link Trigger} that gets {@link #run() run}
      * periodically. This is useful when your trigger does
      * some polling work.
      */
-    protected Trigger(String cronTabSpec) throws ANTLRException {
+    protected Trigger(@Nonnull String cronTabSpec) throws ANTLRException {
         this.spec = cronTabSpec;
         this.tabs = CronTabList.create(cronTabSpec);
     }
@@ -316,7 +320,7 @@ public static DescriptorExtensionList<Trigger<?>,TriggerDescriptor> all() {
      * Returns a subset of {@link TriggerDescriptor}s that applys to the given item.
      */
     public static List<TriggerDescriptor> for_(Item i) {
-        List<TriggerDescriptor> r = new ArrayList<TriggerDescriptor>();
+        List<TriggerDescriptor> r = new ArrayList<>();
         for (TriggerDescriptor t : all()) {
             if(!t.isApplicable(i))  continue;
 

diff --git a/core/src/main/java/hudson/util/SequentialExecutionQueue.java b/core/src/main/java/hudson/util/SequentialExecutionQueue.java
@@ -1,5 +1,6 @@
 package hudson.util;
 
+import javax.annotation.Nonnull;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
@@ -58,7 +59,7 @@ public synchronized void setExecutors(ExecutorService svc) {
     }
 
 
-    public synchronized void execute(Runnable item) {
+    public synchronized void execute(@Nonnull Runnable item) {
         QueueEntry e = entries.get(item);
         if(e==null) {
             e = new QueueEntry(item);

diff --git a/core/src/main/java/jenkins/triggers/ReverseBuildTrigger.java b/core/src/main/java/jenkins/triggers/ReverseBuildTrigger.java
@@ -72,6 +72,8 @@
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.QueryParameter;
 
+import javax.annotation.Nonnull;
+
 /**
  * Like {@link BuildTrigger} but defined on the downstream project.
  * Operates via {@link BuildTrigger#execute} and {@link DependencyGraph},
@@ -84,12 +86,13 @@
 public final class ReverseBuildTrigger extends Trigger<Job> implements DependencyDeclarer {
 
     private static final Logger LOGGER = Logger.getLogger(ReverseBuildTrigger.class.getName());
-    private static final Map<Job,Collection<ReverseBuildTrigger>> upstream2Trigger = new WeakHashMap<Job,Collection<ReverseBuildTrigger>>();
+    private static final Map<Job,Collection<ReverseBuildTrigger>> upstream2Trigger = new WeakHashMap<>();
 
     private String upstreamProjects;
     private final Result threshold;
 
-    @DataBoundConstructor public ReverseBuildTrigger(String upstreamProjects, Result threshold) {
+    @DataBoundConstructor
+    public ReverseBuildTrigger(String upstreamProjects, Result threshold) {
         this.upstreamProjects = upstreamProjects;
         this.threshold = threshold;
     }
@@ -155,7 +158,7 @@ private boolean shouldTrigger(Run upstreamBuild, TaskListener listener) {
                 synchronized (upstream2Trigger) {
                     Collection<ReverseBuildTrigger> triggers = upstream2Trigger.get(upstream);
                     if (triggers == null) {
-                        triggers = new LinkedList<ReverseBuildTrigger>();
+                        triggers = new LinkedList<>();
                         upstream2Trigger.put(upstream, triggers);
                     }
                     triggers.remove(this);
@@ -222,14 +225,14 @@ public FormValidation doCheckUpstreamProjects(@AncestorInPath Job project, @Quer
     }
 
     @Extension public static final class RunListenerImpl extends RunListener<Run> {
-        @Override public void onCompleted(Run r, TaskListener listener) {
+        @Override public void onCompleted(@Nonnull Run r, @Nonnull TaskListener listener) {
             Collection<ReverseBuildTrigger> triggers;
             synchronized (upstream2Trigger) {
                 Collection<ReverseBuildTrigger> _triggers = upstream2Trigger.get(r.getParent());
                 if (_triggers == null || _triggers.isEmpty()) {
                     return;
                 }
-                triggers = new ArrayList<ReverseBuildTrigger>(_triggers);
+                triggers = new ArrayList<>(_triggers);
             }
             for (final ReverseBuildTrigger trigger : triggers) {
                 if (trigger.shouldTrigger(r, listener)) {

diff --git a/core/src/test/java/hudson/triggers/SCMTriggerTest.java b/core/src/test/java/hudson/triggers/SCMTriggerTest.java
@@ -0,0 +1,42 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2015 Kanstantsin Shautsou
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package hudson.triggers;
+
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+
+/**
+ * @author Kanstantsin Shautsou
+ */
+public class SCMTriggerTest {
+    @Issue({"JENKINS-29790", "JENKINS-29945"})
+    @Test
+    public void testNoNPE() throws Exception {
+        final SCMTrigger scmTrigger = new SCMTrigger("");
+
+        scmTrigger.run();
+        scmTrigger.run(null);
+        scmTrigger.getProjectActions();
+    }
+}
diff --git a/core/src/test/java/hudson/triggers/TimerTriggerTest.java b/core/src/test/java/hudson/triggers/TimerTriggerTest.java
@@ -0,0 +1,39 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2015 Kanstantsin Shautsou
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package hudson.triggers;
+
+import antlr.ANTLRException;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+
+/**
+ * @author Kanstantsin Shautsou
+ */
+public class TimerTriggerTest {
+    @Issue("JENKINS-29790")
+    @Test
+    public void testNoNPE() throws ANTLRException {
+        new TimerTrigger("").run();
+    }
+}
diff --git a/pom.xml b/pom.xml
@@ -101,6 +101,7 @@ THE SOFTWARE.
     <matrix-project.version>1.4.1</matrix-project.version>
     <animal.sniffer.skip>${skipTests}</animal.sniffer.skip>
     <findbugs-maven-plugin.version>3.0.1</findbugs-maven-plugin.version>
+    <test-annotations.version>1.2</test-annotations.version>
 
     <java.level>7</java.level>
   </properties>
@@ -243,7 +244,7 @@ THE SOFTWARE.
     <dependency>
       <groupId>org.jenkins-ci</groupId>
       <artifactId>test-annotations</artifactId>
-      <version>1.1</version>
+      <version>${test-annotations.version}</version>
       <scope>test</scope>
     </dependency>
   </dependencies>

diff --git a/test/pom.xml b/test/pom.xml
@@ -108,9 +108,9 @@ THE SOFTWARE.
     <dependency>
       <groupId>org.jenkins-ci</groupId>
       <artifactId>test-annotations</artifactId>
-      <version>1.1</version>
-      <scope>compile</scope>
+      <version>${test-annotations.version}</version>
       <!-- in this module we need this as a compile scope, whereas in the parent it's test -->
+      <scope>compile</scope>
     </dependency>
     <dependency>
       <groupId>org.jvnet.mock-javamail</groupId>

diff --git a/test/src/test/java/hudson/PluginTest.java b/test/src/test/java/hudson/PluginTest.java
@@ -34,7 +34,7 @@ public class PluginTest {
 
     @Rule public JenkinsRule r = new JenkinsRule();
 
-    @Issue("SECURITY-131") // TODO test-annotations 1.2+: @Issue({"SECURITY-131", "SECURITY-155"})
+    @Issue({"SECURITY-131", "SECURITY-155"})
     @Test public void doDynamic() throws Exception {
         r.createWebClient().goTo("plugin/credentials/images/24x24/credentials.png", "image/png");
         /* Collapsed somewhere before it winds up in restOfPath: