[JENKINS-43653] - Ensure AbstractItem#delete() NPE safety when checki…

…ng executors (#2854) * [JENKINS-43653] - Ensure AbstractItem#delete() NPE and RTE safety when checking executors. This change adds missing NPE checks and also improves handling of Executables#getParentOf(), which may throw undocumented Runtime exceptions if Executable uses core API below 1.377 and does not implement getParent(). Executor#stop() has been also modified to explicitly handle the issue though there is still the same issue with estimated execution times. * [JENKINS-43653] - Address comments from @jglick, restrict newly introduced API * [JENKINS-43653] - Address comments from @stephenc * [JENKINS-43653] - Cleanup "unrelevant" changes to make @stephenc and @jglick happy * [JENKINS-43653] - Cleanup the leftover * [JENKINS-43653] - Drop the changes in Executables
jenkinsci · Apr 23, 2017 · 132ca7c · 132ca7c
1 parent cdb45dd
commit 132ca7c
Show file tree

Hide file tree

Showing 4 changed files with 18 additions and 5 deletions.
diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java
@@ -31,6 +31,7 @@
 import hudson.Functions;
 import hudson.BulkChange;
 import hudson.cli.declarative.CLIResolver;
+import hudson.model.Queue.Executable;
 import hudson.model.listeners.ItemListener;
 import hudson.model.listeners.SaveableListener;
 import hudson.model.queue.Tasks;
@@ -87,6 +88,8 @@
 import javax.xml.transform.stream.StreamSource;
 
 import static hudson.model.queue.Executables.getParentOf;
+import hudson.model.queue.SubTask;
+import java.lang.reflect.InvocationTargetException;
 import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
 import org.apache.commons.io.FileUtils;
 import org.kohsuke.accmod.Restricted;
@@ -620,9 +623,12 @@ public void delete() throws IOException, InterruptedException {
                 Map<Executor, Queue.Executable> buildsInProgress = new LinkedHashMap<>();
                 for (Computer c : Jenkins.getInstance().getComputers()) {
                     for (Executor e : c.getAllExecutors()) {
-                        WorkUnit workUnit = e.getCurrentWorkUnit();
-                        if (workUnit != null) {
-                            Item item = Tasks.getItemOf(getParentOf(workUnit.getExecutable()));
+                        final WorkUnit workUnit = e.getCurrentWorkUnit();
+                        final Executable executable = workUnit != null ? workUnit.getExecutable() : null;
+                        final SubTask subtask = executable != null ? getParentOf(executable) : null;
+
+                        if (subtask != null) {        
+                            Item item = Tasks.getItemOf(subtask);
                             if (item != null) {
                                 while (item != null) {
                                     if (item == this) {

diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java
@@ -511,6 +511,7 @@ public void completedAsynchronous(@CheckForNull Throwable error) {
      *      null if the executor is idle.
      */
     @Exported
+    @CheckForNull
     public WorkUnit getCurrentWorkUnit() {
         lock.readLock().lock();
         try {

diff --git a/core/src/main/java/hudson/model/queue/Executables.java b/core/src/main/java/hudson/model/queue/Executables.java
@@ -36,10 +36,14 @@
  * @author Kohsuke Kawaguchi
  */
 public class Executables {
+
     /**
-     * Due to the return type change in {@link Executable}, the caller needs a special precaution now.
+     * Due to the return type change in {@link Executable} in 1.377, the caller needs a special precaution now.
+     * @param e Executable
+     * @return Discovered subtask
      */
-    public static @Nonnull SubTask getParentOf(Executable e) {
+    public static @Nonnull SubTask getParentOf(@Nonnull Executable e) 
+            throws Error, RuntimeException {
         try {
             return e.getParent();
         } catch (AbstractMethodError _) {
@@ -77,6 +81,7 @@ public static long getEstimatedDurationFor(@CheckForNull Executable e) {
         try {
             return e.getEstimatedDuration();
         } catch (AbstractMethodError error) {
+            // TODO: according to the code above, e.getparent() may fail. The method also needs to be reworked
             return e.getParent().getEstimatedDuration();
         }
     }

diff --git a/core/src/main/java/hudson/model/queue/WorkUnit.java b/core/src/main/java/hudson/model/queue/WorkUnit.java
@@ -79,6 +79,7 @@ public void setExecutor(@CheckForNull Executor e) {
     /**
      * If the execution has already started, return the executable that was created.
      */
+    @CheckForNull
     public Executable getExecutable() {
         return executable;
     }