Validate missing memory #317
Conversation
axic
force-pushed
the
validate-missing-memory
branch
2 times, most recently
from
50692ce
to
099ff6a
Compare
|
Need to add a unit test before merging. |
axic
force-pushed
the
validate-missing-memory
branch
from
099ff6a
to
c2024e8
Compare
axic
force-pushed
the
validate-missing-memory
branch
from
c2024e8
to
f98658d
Compare
test/unittests/parser_test.cpp
Outdated
| EXPECT_THROW_MESSAGE(parse(bin), parser_error, "unexpected memidx value 1"); | ||
| } | ||
|
|
||
| TEST(parser, DISABLED_empty_data_section_without_memory) | ||
| { | ||
| const auto bin = bytes{wasm_prefix} + make_section(11, make_vec({})); |
There was a problem hiding this comment.
Since empty data sections end up being datasec.empty() in the parser, this case is not detected.
@gumb0 did you say the spec states an empty data section can or cannot be present if no memory section is present?
There was a problem hiding this comment.
It cannot. Any data section is invalid if no memory is present, according to validation spec.
There was a problem hiding this comment.
Any suggestion how to clearly support that in our code? Since section kind's are strictly ordered (and we enforce that already), the best option seems to be moving the check into the switch statement right before parse_vec<Data>. What do you think?
There was a problem hiding this comment.
It requires info about imports, so to move data and element section checks into the switch, you would need to move "split imports" loop inside case SectionId::import, too.
I'm fine in this PR to leave like it is now.
There was a problem hiding this comment.
The current check covers empty data section, too, doesn't it? So this test can be just enabled?
There was a problem hiding this comment.
The current check covers empty data section, too, doesn't it? So this test can be just enabled?
It does not, if it is enabled it fails. You can try yourself and perhaps come up with a clever solution.
There was a problem hiding this comment.
Ah, I misunderstood what is '"empty data section", I thought it's "data section with empty data vector" (that would be invalid), but you mean "section consisting of empty vector". I can't find any restriction for a data section as a whole, I guess any section with empty contents is allowed.
Also, in binary spec it says:
There was a problem hiding this comment.
Good find, will take your change then.
axic
force-pushed
the
validate-missing-memory
branch
from
f98658d
to
92789f8
Compare
axic
force-pushed
the
validate-missing-memory
branch
from
92789f8
to
0ea3f2a
Compare
Codecov Report
@@ Coverage Diff @@
## master #317 +/- ##
=======================================
Coverage 99.11% 99.11%
=======================================
Files 38 38
Lines 11162 11176 +14
=======================================
+ Hits 11063 11077 +14
Misses 99 99 |
No description provided.