Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-43653] - Ensure AbstractItem#delete() NPE safety when checking executors #2854

Merged

Conversation

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Apr 22, 2017

This is a regression introduced by #2789 in 2.55. See See JENKINS-43653.

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.

No autotests since the fix is expedited to 2.56.

Changelog entries

Proposed changelog entries:

  • JENKINS-43653: Ensure NullPointerException safety when aborting builds during item deletion (regression in 2.55).
  • ...

Submitter checklist

  • JIRA issue is well described
  • Link to JIRA ticket in description, if appropriate
  • Appropriate autotests or explanation to why this change has no tests
  • For new API and extension points: Link to the reference implementation in open-source (or example in Javadoc)

Desired reviewers

@reviewbybees @stephenc

…n 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.
@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Apr 22, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@@ -620,9 +621,10 @@ 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()));

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 22, 2017

Author Member

The reported issue is getParentOf

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Apr 22, 2017

Bit overly finicky of an impl

Copy link
Member

jglick left a comment

I am not fully comfortable with the new API methods in Executables; at least they should be @Restricted(NoExternalUse.class) for purposes of a hotfix. The Executor change also looks gratuitous. The actual issue in AbstractItem.delete could have been addressed much more conservatively with a single try-catch.

try {
parentOf = getParentOfOrFail(executable);
} catch(InvocationTargetException ex) {
return HttpResponses.error(500, ex);

This comment has been minimized.

Copy link
@jglick

jglick Apr 22, 2017

Member

Suffices to just throw this exception (or a wrapper) out of the method.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 22, 2017

Author Member

This is a non-Restricted public API (it should be restricted, of course), hence I would prefer to keep the current implementation if you do not feel strongly. The user-visible behavior does not change

This comment has been minimized.

Copy link
@jglick

jglick Apr 22, 2017

Member

Executor.doStop is actually used from plugins AFAIK, so should not be restricted. Better to leave its original implementation alone unless this is somehow required to fix the reported bug.

@CheckForNull
public static SubTask getParentOfOrNull(@CheckForNull Executable e) {
if (e == null) {
return null;

This comment has been minimized.

Copy link
@jglick

jglick Apr 22, 2017

Member

Please do not introduce API methods accepting null arguments for no particular reason. Should be @Nonnull.

public static SubTask getParentOfOrFail(@Nonnull Executable e) throws InvocationTargetException {
try {
return getParentOf(e);
} catch(RuntimeException | Error ex) {

This comment has been minimized.

Copy link
@jglick

jglick Apr 22, 2017

Member

AbstractMethodError you mean?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 22, 2017

Author Member

Nope. It calls the getParentOf()method above, which throws these types :(

* Get parent subtask from which the executable has been created.
* @param e Executable
* @return Parent subtask from which the executable has been created
* @throws InvocationTargetException Operation failure due to the usage of incompatible API for old plugin depending on a core older than 1.377

This comment has been minimized.

Copy link
@jglick

jglick Apr 22, 2017

Member

I do not think we need a new method here. Suffices to document the exception in the original, and have doStop let it be thrown up as it did before—right?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 22, 2017

Author Member

Restricted it for now

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 22, 2017

I am not fully comfortable with the new API methods in Executables; at least they should be @restricted(NoExternalUse.class) for purposes of a hotfix.

The original issue is in Weekly, backporting of the fixes is not required. I would rather prefer to have a "proper" public API, because plugins are supposed to use this Executables utility class to retrieve the tasks instead of directly calling getParent() to be compatible with plugins targeting all cores. Do they do it? Sometimes.

Anyway, I doubt it would be sane to introduce such interface changes again.

Will restrict calls now to get the stuff integrated

Copy link
Member

stephenc left a comment

I think this needs rework on more considered review

@@ -88,6 +89,7 @@

import static hudson.model.queue.Executables.getParentOfOrNull;
import hudson.model.queue.SubTask;
import java.util.Optional;

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 22, 2017

Member

Unused afaiks

import static hudson.model.queue.Executables.getParentOf;
import static hudson.model.queue.Executables.getParentOfOrNull;
import hudson.model.queue.SubTask;
import java.util.Optional;

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 22, 2017

Member

Unused import from what I can see

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 22, 2017

Author Member

Yep, my bad

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 ? getParentOfOrNull(executable) : null;

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 22, 2017

Member

I'm really not liking the OrNull pattern.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 22, 2017

Author Member

OK, will just kill that method and use try/catch

*/
public static @Nonnull SubTask getParentOf(Executable e) {
public static @Nonnull SubTask getParentOf(@Nonnull Executable e)

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 22, 2017

Member

I think the correct thing is to just return e in case of those issues and remove all the junk. I'd need to confirm on other than my phone though

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Apr 22, 2017

I think this is complete over-kill IMHO

Explain why we need any more than

diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java
index 974a940d54..0f44f79f34 100644
--- a/core/src/main/java/hudson/model/AbstractItem.java
+++ b/core/src/main/java/hudson/model/AbstractItem.java
@@ -622,7 +622,8 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet
                     for (Executor e : c.getAllExecutors()) {
                         WorkUnit workUnit = e.getCurrentWorkUnit();
                         if (workUnit != null) {
-                            Item item = Tasks.getItemOf(getParentOf(workUnit.getExecutable()));
+                            Queue.Executable exe = workUnit.getExecutable();
+                            Item item = exe != null ? Tasks.getItemOf(getParentOf(exe)) : null;
                             if (item != null) {
                                 while (item != null) {
                                     if (item == this) {

?

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 22, 2017

Explain why we need any more than

  • workUnit.getExecutable() may return null, will cause NPE in getParentOf
  • getParentOf() may throw exceptions in a edge case (plugin implementing old Executable API)
@stephenc

This comment has been minimized.

How could this ever be? Task extends SubTask, and 1.376 (and before) had Task getParent() the error catching are just because we have to do something. Show evidence that these catch clauses could ever be required

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Apr 22, 2017

The bug here is that I forgot the race condition where there is a delay between assigning the work unit and assigning the executable. That bug is all we should be fixing here.

GetParentOf will never throw exceptions. If you analyse the pre-1.377 signature and the post 1.377 signature you will see that no implementation will ever actually throw an exception. javac cannot know what the old signature was and hence does not see that there will never be an exception, hence it requires that we throw something.

Now with Java 9 and modules, we could end up with exceptions... except old code cannot be using modules so will be in the unnamed module so we will be able to setAccessible anyway and in any case we would need to be using modules for the plugin classloader to even have to worry about this.

Just go for the minimalist fix here, the extra API stuff is wholesale unnecessary IMHO (but I will. E pleasantly surprised if you can show my analysis incorrect)

Copy link
Member

jglick left a comment

From looking at the JIRA issue, all that is required is a simple null check, as in @stephenc’s proposed patch. From mobile I lack the tools to understand the apparently unrelated hypothetical problems you are trying to address in this PR.

* @param e Executable
* @return Parent subtask from which the executable has been created
* @throws InvocationTargetException Operation failure due to the usage of incompatible API for old plugin depending on a core older than 1.377
* @since TODO

This comment has been minimized.

Copy link
@jglick

jglick Apr 22, 2017

Member

If restricting, delete @since.

try {
parentOf = getParentOfOrFail(executable);
} catch(InvocationTargetException ex) {
return HttpResponses.error(500, ex);

This comment has been minimized.

Copy link
@jglick

jglick Apr 22, 2017

Member

Executor.doStop is actually used from plugins AFAIK, so should not be restricted. Better to leave its original implementation alone unless this is somehow required to fix the reported bug.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 22, 2017

No time to continue arguing this weekend. Will remove everything excepting the NPE fix & follow-up on other things afterwards

@oleg-nenashev oleg-nenashev changed the title [JENKINS-43653] - Ensure AbstractItem#delete() NPE and RTE safety when checking executors [JENKINS-43653] - Ensure AbstractItem#delete() NPE safety when checking executors Apr 22, 2017
@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 23, 2017

@stephenc E.g. there may be a problem with old version of DistFork plugin: https://issues.jenkins-ci.org/browse/JENKINS-8100 . Unrealistic example, of course. Will investigate on the next week, removing the code right now

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 23, 2017

I have left the TODO and Javadoc comment for Executables + Annotations. But there is no code changes left for that exception, only the NPE fix

Copy link
Member

stephenc left a comment

Recommend squashing before merge. I don't like adding rumours of JENKINS-8100 without confirmation

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Apr 23, 2017

JENKINS-8106 fixed JENKINS-8100 please drop the TODO noise

* 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
* @throws Error Executable type does not offer the {@link Executable#getParent()} method or it fails with {@link Error}.

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 23, 2017

Member

Noise see JENKINS-8106

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Apr 23, 2017

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Apr 23, 2017

Or dced4d4 may be the commit and it is https://issues.jenkins-ci.org/plugins/servlet/mobile#issue/JENKINS-7546 (which was fixed in 1.380 rather than 1.388)

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Apr 23, 2017

In either case the getParentOf method was to ensure correct signature handling and those TODOs are just noise.

I am fine with the two @CheckForNull annotations though

@jglick
jglick approved these changes Apr 23, 2017
final Executable executable = workUnit != null ? workUnit.getExecutable() : null;
final SubTask subtask = executable != null ? getParentOf(executable) : null;

if (subtask != null) {

This comment has been minimized.

Copy link
@jglick

jglick Apr 23, 2017

Member

Could be simplified to only check executable != null.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 23, 2017

@stephenc OK, dropped it. The PR will be squashed of course

Copy link
Member

stephenc left a comment

Looks fine, only minor niggles

@@ -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

This comment has been minimized.

Copy link
@stephenc
*/
public static @Nonnull SubTask getParentOf(Executable e) {
public static @Nonnull SubTask getParentOf(@Nonnull Executable e)
throws Error, RuntimeException {

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 23, 2017

Member

Throws are unnecessary

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2017

Member

Agreed with other reviewers, these throws should be reverted.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 23, 2017

@reviewbybees done
@jenkinsci/code-reviewers I would appreciate authorization to merge it (regression in the last weekly). IMHO it is important to get it in this weekly

*/
public static @Nonnull SubTask getParentOf(Executable e) {
public static @Nonnull SubTask getParentOf(@Nonnull Executable e)
throws Error, RuntimeException {

This comment has been minimized.

Copy link
@mc1arke

mc1arke Apr 23, 2017

Member

No need to declare a method throwing a RuntimeException

Copy link
Member

daniel-beck left a comment

LTGM assuming squash.

@@ -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;

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Apr 23, 2017

Member

Unused import?

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 23, 2017

PR builder passed. Merging it, will cleanup the leftover after the weekly

@oleg-nenashev oleg-nenashev merged commit 132ca7c into jenkinsci:master Apr 23, 2017
2 checks passed
2 checks passed
Jenkins This pull request looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.