Skip to content

Conversation

@kripken
Copy link
Member

@kripken kripken commented Jan 12, 2022

When ignoring external input, assume params have a value of 0. This
makes it possible to eval main(argc, argv) if one is careful and does
not actually use those values.

This is basically a workaround for main always receiving argc/argv,
even if the C code has no args (in that case the compiler emits
__original_main for the user's main, and wraps it with a main
that adds the args, hence the problem).

This is similar to the existing support for handling wasi_args_get
when ignoring external input, although it just sets values of zeros for
the params. Perhaps it could check for main() specifically and return
1 for argc and a proper buffer for argv somehow, but I think if a program
wants to use --ignore-external-input it can avoid actually reading
argc/argv.

@kripken kripken requested review from sbc100 and tlively January 12, 2022 17:17
@tlively
Copy link
Member

tlively commented Jan 12, 2022

Ignoring external parameters when that flag is explicitly set makes sense, but we could also ignore external parameters that are unused even when that flag is not set. Would that be sufficient for the case of main?

@kripken
Copy link
Member Author

kripken commented Jan 12, 2022

I think that would be sufficient for main, yes. It would be significantly more work, though - we'd need to track local usage.

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps worth a TODO about ignoring unused parameters since --ignore-external-input seems extremely dangerous.

@sbc100
Copy link
Member

sbc100 commented Jan 12, 2022

I'm fine with this land now but I think it would be good if we could be more specific and make this work even without --ignore-external-input. We already detect mainReadParams elsewhere in binaryen so we should be able to do a similar thing here.

I also thinks its fine if call out main explicitly if it makes sense.. and also isn't it fairly easy to see if a local is used or not?

@kripken
Copy link
Member Author

kripken commented Jan 12, 2022

Yeah, it's fairly easy to see if a local is used statically. Dynamic checks as the interpreter runs would require more work, which is what I meant earlier. Static checks would be good enough for the case where there is no reference at all, but we'd need dynamic checks to be able to partially eval up to the first usage, to ignore local.gets in code paths that are not taken, etc.

Overall I think maybe we want more than one option here. One issue is that assuming parameters are zero is actually nice for the fuzzer (it's literally what the fuzzer does when it tests, so it "just works"), where I've just been running with --ignore-external-input all the time. But yes, in general for users maybe we want something safer than that. Also maybe we should have separate flags for the environment, for params, maybe even for specific WASI APIs. I don't have a good idea of the final point there.

So yes, for now I think let's land this with a TODO, and we'll think about more refined and safer options later.

@kripken kripken enabled auto-merge (squash) January 12, 2022 21:01
@kripken kripken merged commit 29604f1 into main Jan 12, 2022
@kripken kripken deleted the ceparams branch January 12, 2022 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants