Skip to content
This repository has been archived by the owner on Dec 15, 2021. It is now read-only.

[JENKINS-32925] Add tracking to prevent stack overflows #351

Merged
merged 4 commits into from Mar 4, 2016
Merged

[JENKINS-32925] Add tracking to prevent stack overflows #351

merged 4 commits into from Mar 4, 2016

Conversation

kwhetstone
Copy link
Contributor

JENKINS-32925

To prevent stack overflows, especially in the Multiple SCMs case, track the subtypes to make sure the type isn't trying to discover itself. When that case is detected, don't log, and return back up the stack. This will only prevent duplication vertically, not horizontally.

@reviewbybees esp @jglick & @abayer

@ghost
Copy link

ghost commented Mar 3, 2016

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.

@abayer
Copy link
Member

abayer commented Mar 3, 2016

Oooo, nicely done. 🐝

@@ -147,6 +148,10 @@
public static Schema schemaFor(Class<?> clazz) {
return new Schema(clazz);
}

public static Schema schemaFor(Class<?> clazz, Stack<String> tracker) {
Copy link
Member

Choose a reason for hiding this comment

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

🐛 should be private

@jglick
Copy link
Member

jglick commented Mar 3, 2016

Good enough for the short term: addresses the stack overflow by simply omitting a legitimate choice. Later, if needed, we can introduce a new parameter type like Error which explicitly indicates recursion (or even a horizontal backreference), but that would require changes to the content generators too.

@jglick
Copy link
Member

jglick commented Mar 3, 2016

BTW there is an open issue for this on issues.jenkins-ci.org, assigned to you IIRC; link to it please.

@jglick jglick changed the title Add tracking to prevent stack overflows. [JENKINS-32925] Add tracking to prevent stack overflows Mar 3, 2016
this.type = clazz;
if(tracker == null){
tracker = new Stack<String>();
}
Copy link
Member

Choose a reason for hiding this comment

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

Or more simply, pass new Stack<String>() as the argument in line 166.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the @Nonnull annotation to the Stack in the constructor. I really like that annotation.

@kwhetstone
Copy link
Contributor Author

Thanks for the reviews. I wanted to make a fairly minimal change so there wouldn't be that huge disruption to all the content generators. I definitely will keep introducing the error type in the back of my mind if this ends up being needed down the road.

@jglick
Copy link
Member

jglick commented Mar 3, 2016

🐝

@jglick jglick merged commit 64e8b85 into jenkinsci:master Mar 4, 2016
jglick added a commit that referenced this pull request Mar 4, 2016
@kohsuke
Copy link
Member

kohsuke commented Mar 29, 2016

While studying this fix, I noticed that this change missed the use of of in ArrayType instantiation here so the stack overflow continues with multiple-scm plugin.

The recursive portion of the stack is as follows (as of c85a422):

    at org.jenkinsci.plugins.workflow.structs.DescribableHelper$Schema.<init>(DescribableHelper.java:179)
    at org.jenkinsci.plugins.workflow.structs.DescribableHelper.schemaFor(DescribableHelper.java:152)
    at org.jenkinsci.plugins.workflow.structs.DescribableHelper.access$400(DescribableHelper.java:77)
    at org.jenkinsci.plugins.workflow.structs.DescribableHelper$ParameterType.of(DescribableHelper.java:344)
    at org.jenkinsci.plugins.workflow.structs.DescribableHelper$ParameterType.of(DescribableHelper.java:298)
    at org.jenkinsci.plugins.workflow.structs.DescribableHelper$ParameterType.of(DescribableHelper.java:369)
    at org.jenkinsci.plugins.workflow.structs.DescribableHelper$ParameterType.access$200(DescribableHelper.java:285)
    at org.jenkinsci.plugins.workflow.structs.DescribableHelper$Schema.<init>(DescribableHelper.java:179)

@jglick
Copy link
Member

jglick commented Mar 29, 2016

Probably better to leave a comment in jenkinsci/structs-plugin#1 requesting a test case be added that has something like Impl3 but whose polymorphic attribute is of type Base[].

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants