Add declaration limit checks to parser#2736
Conversation
| @@ -1620,7 +1634,7 @@ Result WastParser::ParseTagModuleField(Module* module) { | |||
| EXPECT(Lpar); | |||
| EXPECT(Tag); | |||
| Location loc = GetLocation(); | |||
There was a problem hiding this comment.
Usually the location refers to the field type token, not the token after it. Perhaps this is a mistake.
src/wast-parser.cc
Outdated
| size_t size, | ||
| const char* decl) { | ||
| if (size >= kInvalidIndex) { | ||
| Error(loc, "Too many %s declarations", decl); |
There was a problem hiding this comment.
I think the convention is not to start error messages with a capital letter like this.
| Result WastParser::CheckDeclarationLimit(Location& loc, | ||
| size_t size, | ||
| const char* decl) { | ||
| if (size >= kInvalidIndex) { |
There was a problem hiding this comment.
I guess there are likely implemenation limits that are much lower than 2^32 that we could apply here?
With this check we are going to get OOM way before we hit it on 32-bit platforms right?
There was a problem hiding this comment.
Yes, on 32 bit platforms this cannot happen. What do you suggest? A 64 bit specific macro? Or this patch is a bad idea?
There was a problem hiding this comment.
I think this patch is good, but we we might also want to enforce much lower limits such as the ones defined in https://webassembly.github.io/spec/core/appendix/implementation.html#implementation-limitations.
That doesn't have to happen as part of this PR though.
|
Changed the capital letter. |
include/wabt/wast-parser.h
Outdated
| Result Synchronize(SynchronizeFunc); | ||
|
|
||
| // Check the maximum allowed declarations. | ||
| Result CheckDeclarationLimit(Location& loc, size_t size, const char* decl); |
There was a problem hiding this comment.
Maybe we can come up with a better name? Maybe CheckIndexRange, since we are verifying that its doesn't exceed the size of Index?
2 participants
The "focus on correctness" in #2735 made me thinking. This was something I was aware for a longer time, but never did anything to it. The binary reader reads the number of items from the file, so the maximum amount of declarations is always less or equal than kInvalidIndex (equals to the maximum uint32_t value). These declarations are indexed from
[0 .. kInvalidIndex - 1]. As far as I see there is no such limit for the text parser. This patch adds some limit checks.