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-44942] - FilePath.list() & .listDirectories() null safety #2914

Merged
merged 1 commit into from Jun 16, 2017

Conversation

jglick
Copy link
Member

@jglick jglick commented Jun 12, 2017

Description

No known NPE resulting from this, but better to be on the safe side.

https://issues.jenkins-ci.org/browse/JENKINS-44942

Changelog entries

Proposed changelog entries:

  • JENKINS-44942 - Prevent possible NullPointerException resulting when listing remote directories using the FilePath#list() and FilePath#ListDirectories() API.

Desired reviewers

@reviewbybees

…was not in fact so.

FilePath.list() did not specify, but at least DownloadFromUrlInstaller assumed it was non-null.
@ghost
Copy link

ghost commented Jun 12, 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.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be also great to catch the SecurityException from ListFiles() then. Similarly to jenkinsci/periodicbackup-plugin@466393e

🐝 anyway though it is not full fix

@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Jun 12, 2017
@jglick
Copy link
Member Author

jglick commented Jun 12, 2017

It would be also great to catch the SecurityException from ListFiles() then

Does not make sense to me. There should not be a SecurityManager set on the Jenkins master or agents, or if there is, it must not deny file reads.

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Jun 12, 2017

There should not be a SecurityManager set on the Jenkins master or agents....

Not documented anywhere. I also do not see a reason for such no-go answer. Security managers are being widely used on security-paranoid enterprise instances.

, or if there is, it must not deny file reads.

Likely yes. The problem is what's going to happen if it does, we may get into undefined behavior due to the runtime exceptions (hanging agents, file leaks, etc.). But I'd guess FilePath is not the first thing to worry about if we are really aware about SecurityException handling in Jenkins.

Anyway, I have approved the PR

@jglick
Copy link
Member Author

jglick commented Jun 12, 2017

Security managers are being widely used on security-paranoid enterprise instances.

Huh?? You would to patch Jenkins sources to install a security manager. And you will very likely break all kinds of things in the process. Jenkins is designed to assume it has full control over the JVM.

@jglick
Copy link
Member Author

jglick commented Jun 13, 2017

@reviewbybees done

@oleg-nenashev oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Jun 15, 2017
@oleg-nenashev oleg-nenashev changed the title FilePath.list() & .listDirectories() null safety [JENKINS-44942] - FilePath.list() & .listDirectories() null safety Jun 16, 2017
@oleg-nenashev
Copy link
Member

Created issue placeholder just in case we decide to backport it: https://issues.jenkins-ci.org/browse/JENKINS-44942

@oleg-nenashev oleg-nenashev merged commit a5dc255 into jenkinsci:master Jun 16, 2017
@jglick jglick deleted the FilePath.list branch June 16, 2017 11:07
olivergondza pushed a commit that referenced this pull request Jul 4, 2017
[JENKINS-44942] - FilePath.list() & .listDirectories() null safety

(cherry picked from commit a5dc255)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
2 participants