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

Create ClassLoaders inside doPrivileged() + Auto-close the WAR file after exploding it #411

Merged
merged 2 commits into from
Nov 1, 2020

Conversation

recrsn
Copy link
Contributor

@recrsn recrsn commented Oct 27, 2020

Partially addresses #374

@recrsn recrsn requested a review from a team as a code owner October 27, 2020 21:22
@oleg-nenashev oleg-nenashev self-requested a review October 27, 2020 21:52
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.

Should be good though I will need to add a test for the AccessController and WAR resource loading

@@ -338,12 +341,12 @@ public int run() throws Throwable {
}
}

public ClassLoader createJenkinsWarClassLoader() throws IOException, NoSuchMethodException, InvocationTargetException, IllegalAccessException {
Copy link
Member

Choose a reason for hiding this comment

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

I do not recall what led to this exception list

return new ClassLoaderBuilder(new SideClassLoader(getPlatformClassloader()))
.collectJars(new File(warDir,"WEB-INF/lib"))
public ClassLoader createJenkinsWarClassLoader() throws PrivilegedActionException {
return AccessController.doPrivileged((PrivilegedExceptionAction<ClassLoader>) () -> new ClassLoaderBuilder(new SideClassLoader(getPlatformClassloader()))
Copy link
Member

Choose a reason for hiding this comment

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

Jenkins is unlikely to work on advanced security policies . Should be fine though I will need to add Autotests for the resource loading

Copy link
Member

Choose a reason for hiding this comment

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

This is pointless. See spotbugs/spotbugs#1515

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.

I tested it locally to verify the workaround for #413, and I confirm it still works well after the patch

@oleg-nenashev oleg-nenashev merged commit 0b3ef9a into jenkinsci:master Nov 1, 2020
@oleg-nenashev oleg-nenashev added the bug Something isn't working label Nov 1, 2020
@oleg-nenashev oleg-nenashev changed the title Fix some spotbugs warnings Create ClassLoaders inside doPrivileged() + Auto-close JAR file Nov 1, 2020
@oleg-nenashev oleg-nenashev changed the title Create ClassLoaders inside doPrivileged() + Auto-close JAR file Create ClassLoaders inside doPrivileged() + Auto-close the WAR file after exploding it Nov 1, 2020
@recrsn recrsn deleted the fix-spotbugs branch November 2, 2020 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chore Maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants