Restrict the maximum nesting level in the parser to avoid stack overflows #1030
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci Please test |
|
@swift-ci Please test macOS |
DougGregor
approved these changes
Oct 27, 2022
|
|
||
| /// A default maximum nesting level that is used if the client didn't | ||
| /// explicitly specify one. | ||
| public static let defaultMaximumNestingLevel = 256 |
There was a problem hiding this comment.
I know it's terrible, but should we do some kind of #if DEBUG check to decide whether to use the lower limit or higher limit?
There was a problem hiding this comment.
That’s actually a good idea. The entire check is pretty horrible in itself, so a #if DEBUG check doesn’t make a huge difference anymore.
Sources/SwiftParser/TopLevel.swift
Outdated
| @@ -13,6 +13,27 @@ | |||
| @_spi(RawSyntax) import SwiftSyntax | |||
|
|
|||
| extension Parser { | |||
| /// Consumes and returns all remaining tokens in the source file. | |||
| mutating func consumeRemaingingTokens() -> [RawSyntax] { | |||
Sources/SwiftParser/TopLevel.swift
Outdated
|
|
||
| /// If the maximum nesting level has been reached, return the remaining tokens in the source file | ||
| /// as unexpected nodes that have the `isMaximumNestingLevelOverflow` bit set. | ||
| /// Check this in places that are likley to cause deep recursion and if this returns non-nil, abort parsing. |
| || fileURL.absoluteString.contains("_overflow") | ||
| || fileURL.absoluteString.contains("parser-cutoff.swift") | ||
| } | ||
| name: "Swift tests", path: testDir, checkDiagnostics: false |
There was a problem hiding this comment.
This is so fantastic, thank you!
ahoppen
force-pushed
the
ahoppen/prevent-stack-overflow
branch
from
d323cbb to
c3c7599
Compare
|
@swift-ci Please test |
ahoppen
force-pushed
the
ahoppen/prevent-stack-overflow
branch
from
c3c7599 to
213ddc6
Compare
|
@swift-ci Please test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The basic idea is that we start a new nesting level whenever we consume or synthesize an opening bracket (
<,{,(,[or#if) and reduce it when we read the matching closing bracket. This should be a pretty simple indicator for the current nesting level that doesn’t require custom logins in productions. IMO the benefit here is that this notion of nesting level is easy to describe and explain.Productions that are known to be able to overflow (I just took the ones that introduce
StructureMarkerRAIIin the C++ parser) can checkremainingTokensIfMaximumNestingLevelReachedand stop parsing if the maximum nesting level has been exceeded.It turns out that a nesting level of 256 (which is the current maximum nesting level in the C++ parser) is totally fine in Release builds of the parser, but debug builds fail with nesting levels > 30-ish.
Since lookahead cannot really bail out to prevent stack overflow, I rewrote
skipSingleandskipUntilto work iteratively instead of recursively.