Limit number of tokens which can be replayed (reparsed) #1620
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1620 +/- ##
==========================================
+ Coverage 74.73% 74.77% +0.04%
==========================================
Files 288 288
Lines 55340 55408 +68
==========================================
+ Hits 41357 41433 +76
+ Misses 12489 12482 -7
+ Partials 1494 1493 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit 909fd42 Results
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
runtime/parser2/parser.go
Outdated
| @@ -229,13 +231,27 @@ func (p *parser) acceptBuffered() { | |||
| } | |||
| } | |||
|
|
|||
| // replayLimit is a sensible limit for how many tokens may be replayed | |||
| // while parsing a program | |||
| const replayLimit = 2 << 12 | |||
There was a problem hiding this comment.
How did we come up with this value?
It seems like this is setting a total limit on the amount of backtracking for the whole program. Would it not be better to instead impose a (smaller) limit on the amount of backtracking that can happen at once? I.e. impose a limit for each backtracking cursor on the stack instead of a total limit for the stack as a whole?
This has the benefit of being able to localize errors for users should they encounter them; instead of complaining that the entire program is too ambiguous at some arbitrary location where we pass the total limit, we can error at the actual location that has too much backtracking.
There was a problem hiding this comment.
I determined it mostly through experimentation, there is no perfect value. We need to find a limit that rejects invalid programs as quickly as possible, while not rejecting valid programs.
There are problems with both local and global limits. With the global limit, the user may encounter the limit in a locally small ambiguity, because the parser already re-parsed too many tokens before. With the local limit, finding a sensible limit is hard: A small value will reject too many programs, a slightly larger value will not reject invalid programs quickly.
Maybe we can have both local and global limits to balance user experience and security.
There was a problem hiding this comment.
Another idea: Maybe we can enforce the limit locally to each top-level ambiguity, instead of the whole program? We could reset replayedCount in acceptBuffered
There was a problem hiding this comment.
Made the token replay limit "local" to each top-level reparse in f2c81d9. Nested replays count towards the top-most buffering, so errors stay local.
turbolent
force-pushed
the
bastian/limit-reparsing
branch
from
2321e92 to
76e031f
Compare
Description
The parser may reparse parts of the program in cases of ambiguity.
Limit this to a sensible number of tokens. We may want to adjust this limit.
I validated this limit by parsing all Mainnet contracts and all of them remain parsable.
masterbranchFiles changedin the Github PR explorer