Fix handling of data count without data section #2432
Conversation
|
upstreamed WebAssembly/spec#1801 now to make a fix... |
b1cff99
to
559aa39
Compare
| for (Index i = 0; i < num_data_segments; ++i) { | ||
| ERROR_UNLESS( | ||
| data_count_ == kInvalidIndex || data_count_ == num_data_segments_, | ||
| "data segment count does not equal count in DataCount section"); |
There was a problem hiding this comment.
I think this check can not be removed, since the module level check will always catch this error.
There was a problem hiding this comment.
do we want to keep parsing when we encounter a malformed module?
There was a problem hiding this comment.
we think doing this check early is important
There was a problem hiding this comment.
we think doing this check early is important
Can you explain why? It seems redundant to me.
There was a problem hiding this comment.
Whoops, missed this. Uh, we think, because the module is malformed, it's better to exit early if we can. Tho the more important thing is that a malformed module shouldn't crash the parser, we guess.
There was a problem hiding this comment.
I agree the parse should not crash. Are you saying that if you remove these 3 lines the parser will crash? Where/why?
There was a problem hiding this comment.
, because the module is malformed, it's better to exit early if we can.
I think delaying the error would be fine in this case, it it means we can avoid the duplication.
There was a problem hiding this comment.
Tweaked and rebased, let's see what CI has to say.
There was a problem hiding this comment.
well.
based on the changes to the error messages, we prefer the duplicated checks.
914a1dd
to
44205fd
Compare
src/binary-reader.cc
Outdated
| // This must be checked at the end, in case the data section was omitted. | ||
| ERROR_UNLESS( | ||
| data_count_ == kInvalidIndex || data_count_ == num_data_segments_, | ||
| "data segment count does not equal count in DataCount section"); |
There was a problem hiding this comment.
Ok, if we must have a separate check can we change the wording a little so we can tell which error is being emitted.
How about changing this one to be something like: "Non-zero DataCount section, but not data segments found", and maybe surround this check with if (num_data_segments_ == 0) since that is the only time we need to check for this condition.
Closes #2436
Fixes #2310
Fixes #2311
Fixes #2431