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

Prevent StackOverflowException caused by excessive 'or' via PatternMatcher #10330

Merged

Conversation

snazy
Copy link
Contributor

@snazy snazy commented Aug 21, 2019

Fixes #10329

Context

Beside this fixes the java.lang.StackOverflowException described in #10329, it is probably also a slight optimization in the "good case" as it needs way less object instances.

Contributor Checklist

  • Review Contribution Guidelines
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes

@big-guy
Copy link
Member

big-guy commented Aug 21, 2019

@snazy Could you fix the DCO check by signing off your commit? We need this to accept the PR.

Could you create a unit test in PatternMatcherFactoryTest that would trigger the Stackoverflow?

@big-guy big-guy added this to the 6.0 RC1 milestone Aug 21, 2019
@snazy snazy force-pushed the prevent-stack-overflow-for-PatternMatcher branch from 4666345 to d3038df Compare August 22, 2019 18:18
Signed-off-by: Robert Stupp <snazy@snazy.de>
…tcher

Signed-off-by: Robert Stupp <snazy@snazy.de>
@snazy snazy force-pushed the prevent-stack-overflow-for-PatternMatcher branch from d3038df to 1351cac Compare August 22, 2019 18:20
@snazy
Copy link
Contributor Author

snazy commented Aug 22, 2019

Debugging the build that produces the SOE shows that it's triggered by more than 10000 patterns with the stack trace mentioned here.

@big-guy I've added a new unit test to trigger the SOE and also added a kinda-general unit test for org.gradle.api.internal.file.pattern.PatternMatcherFactory#getPatternsMatcher.
The new testNoStackOverflowForManyPatterns fails against vanilla 5.6 and passes with the fix.

Do you have plans for a 5.6.1 release, and could it include this PR?

@big-guy
Copy link
Member

big-guy commented Aug 27, 2019

Thanks @snazy! We'll take a look.

We only allow regression fixes into patch releases to avoid having to do multiple patch releases, so we're not likely to incorporate this into 5.6.1 (which should be any day now).

@big-guy big-guy merged commit 1351cac into gradle:master Sep 4, 2019
@snazy snazy deleted the prevent-stack-overflow-for-PatternMatcher branch September 5, 2019 07:15
@k-brooks
Copy link

@big-guy - could you explain why this is not a regression?
We have a project which compiles successfully in 5.4, upon upgrading to 5.6.2, it exhibits this behavior, it fails to compile. Based on my understanding of the issue, this will likely impact large projects, rendering 5.6 unusable.

@vitgorbunov
Copy link

@big-guy could you please respond here?
I just faced this issue on 5.6.2, dowgraded to 5.5.1 and it's fixed the issue. Indeed I have large project and it will force me to downgrade if fix won't be included in 5.6.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StackOverflowException via PatternMatcher
4 participants