-
Notifications
You must be signed in to change notification settings - Fork 165
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-47758] Script security provides automatic memory leak protection to many groovy scripts #161
Conversation
…y leaks together with cleanup routines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically sounds OK (needs some minor cleanups).
What are you going to do with the original code in workflow-cps
, though? We really do not want to be maintaining two copies of this nightmare indefinitely. Find some way for workflow-cps
to delegate most of the impl to script-security
, with a few tweaks specific to TimingLoader
or whatever.
pom.xml
Outdated
@@ -3,7 +3,7 @@ | |||
<parent> | |||
<groupId>org.jenkins-ci.plugins</groupId> | |||
<artifactId>plugin</artifactId> | |||
<version>2.30</version> | |||
<version>2.35</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2.37 is latest IIRC
@@ -159,23 +307,81 @@ public Object evaluate(ClassLoader loader, Binding binding) throws Exception { | |||
} | |||
try { | |||
loader = GroovySandbox.createSecureClassLoader(loader); | |||
|
|||
Field loaderF = GroovyShell.class.getDeclaredField("loader"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to catch NoSuchFieldException
and quietly fall back gracefully, in case this code is refactored in a future version of Groovy. (I do not recommend swallowing other reflection exceptions—log them as WARNING
s: they are not expected. I.e., do not repeat this mistake: jenkinsci/cloudbees-folder-plugin#79 (comment))
r.buildAndAssertSuccess(p); | ||
|
||
assertFalse(LOADERS.isEmpty()); | ||
{ // TODO it seems that the call to CpsFlowExecutionMemoryTest.register(Object) on a Script1 parameter creates a MetaMethodIndex.Entry.cachedStaticMethod. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is presumably out of date.
@jglick Yes, I'd like to deduplicate the logic -- what you are describing is what I'd hoped to do (modulo some tweaks for extra cleanup that pipeline does). It might require providing some additional hooks for pipeline though because it does additional cleanup. A problem for a later PR though. |
Okay, looks ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it should be effective for most plugins using Groovy. But not all; some use script-security
yet do not use SecureGroovyScript.evaluate
due to various limitations in that API. To claim success we would need to introduce an API in script-security
to perform cleanup, which evaluate
would call but could also be called from other contexts (yes, ideally including workflow-cps
).
try { | ||
loaderF = GroovyShell.class.getDeclaredField("loader"); | ||
loaderF.setAccessible(true); | ||
canDoCleanup = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or simply check loaderF != null
later; then no canDoCleanup
variable is needed.
@jglick Since we know this is having a rather large impact in the wild and fixes 90% of issues, wouldn't it be better to go ahead and release if it looks reasonable, rather that trying to cover every single case? Then we can inform future iterations of this by what we learn from that (for example we may need to broaden it to better cover unanticipated cases with non-pipeline Groovy). Since we have kept the implementation private, we have freedom to change it as needed in order to later expose this in a broader form. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good incremental improvement. Next step is to look for direct calls to GroovySandbox
and figure out how to make the fix available to plugins doing that.
import static org.junit.Assert.assertFalse; | ||
|
||
/** | ||
* @author Sam Van Oort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well…parts. :-) Never believe an @author
tag and delete them when you find them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, okay removing that (autogenerated by IDE, sorry, definitely not trying to claim credit for your hard work)
JENKINS-47758
Snapshot deployed as 1.35-20171101.213128-3 for testing
TODO: