Replace assert statements used for argument validation - #118
Conversation
|
CI failure seems to be unrelated, see #121 |
andreaTP
left a comment
andreaTP
left a comment
There was a problem hiding this comment.
Thanks a lot for submitting this one, a couple of notes, but all the rest lgtm!
| /** | ||
| * Builds the instance. | ||
| * | ||
| * <p>When running in 'runtime compilation' mode, invalid or unsupported Wasm code might already cause an |
There was a problem hiding this comment.
nitpick - I'm a bit confused about this comment, it requires a good understanding of how things are working under the curtains to be able to action it, and usage of the word "might" doesn't help less experienced folks.
There was a problem hiding this comment.
Fair point. My goal was to tell users "expect this to throw any RuntimeException" but without making any guarantees about the details, which seem to be implementation details (?).
But also make it clear that for interpreter mode, this Builder#build() does not (always?) throw an exception for invalid Wasm code but instead that occurs later during the actual execution (at least that is what I experienced).
| case IF: | ||
| case TRY_TABLE: | ||
| assert (scope.isPresent()); | ||
| if (scope.isEmpty()) { |
There was a problem hiding this comment.
for the records: those were asserts in first place as they would detect bugs in the calculation/labelling of the Control Flow instructions.
There was a problem hiding this comment.
Do you mean all of the asserts? Because during fuzzing I encountered a case where assert (labelFalse.isEmpty()); failed. Here is the reproducer (unfortunately not minimal):
AnnotatedInstruction.class.getClassLoader().setDefaultAssertionStatus(true);
byte[] bytes = Base64.getDecoder().decode("AGFzbQEAAAABGAJgB39/f39/f38Bf2AIf39/f39/f38BfwMEAwAAAQcFAQFmAAAKSgMSACAAIAEgAiADIAQgBSAGEAELGwAgACABIAIgA0EfcUGAAXJBACAEUwUgBhICCxkAIAAgAWogApogA2ogBGogBWogBmogB2oL");
Parser.parse(bytes);I guess the Wasm code is invalid, wasm2wat fails for it, but it should nonetheless not cause an AssertionError.
There was a problem hiding this comment.
For that reproducer, maybe the problem is here:
endive/wasm/src/main/java/run/endive/wasm/Parser.java
Lines 1091 to 1100 in 877bdda
It seems for ELSE it just assumes that the currentControlFlow is the corresponding IF (or are there other cases where ELSE is valid?) without actually checking it. But for the reproducer above currentControlFlow is actually LOCAL_GET (the first instruction).
andreaTP
left a comment
andreaTP
left a comment
There was a problem hiding this comment.
LGTM, I took over the last few refinements
Instead of an Exception they cause an AssertionError (an Error), or worse when assertions are disabled at runtime execution just continues despite the program being in an invalid state.
…e#139 - Value.toString(): use safe non-throwing default instead of RuntimeException - Instance.build(): drop confusing Javadoc about interpreter vs compiler modes - Generator: use MalformedException instead of bare RuntimeException - Parser: remove vague class-level Javadoc addition - Emitters.assertTempSlotInRange: replace assert with WasmEngineException
|
Thanks! Regarding #118 (comment); if I see it correctly you completely removed these Javadoc changes again. Are you planning on adding different Javadoc, or will you leave it as it is for now? |
My bad, would you mind sending them back in another PR? |
2 participants
Instead of an
Exceptionthey cause anAssertionError(anError), or worse when assertions are disabled at runtime execution just continues despite the program being in an invalid state.Some of these were found during the fuzzing of #98.
This does not replace all
assertcalls. To me it looks like the remaining ones are not actually used for argument / state validation.