Gracefully handle program rejection from verifier #31
Conversation
Wrap the panic!() invocation in the verifier into a reject() function
which adds the "[Verifier] Error: " prefix to all errors messages. Next
step will be to replace the panic!().
While at it:
- Update one test which expected a non-breakable space in the panic
message ("[Verifier].Error: ...").
- Remove a spurious blank line in the verifier.
Make verifier return a Result<(), String> and handle verification errors gracefully. This means that in src/lib.rs, the new() and set_program() methods must be addapted to propagate the errors. Then update all tests, including in source comments, to unwrap() the virtual machines at their creation.
|
@qmonnet If this change requires a change to the package major version would you also like to get in the replaceable verifier changes as well since that also changes the api? |
|
This crate is at version 0.0.3, so... I guess we shouldn't worry too much about version numbers yet :D |
|
We would like to get an updated crate released by the end of the month that includes both these changes and the replacable verifier. Is that possible? |
|
Hi, I have nothing against updating the crate (as long as we update the README before that, but that should not be difficult). I haven't done it in some time because I did not think anyone would use it, but if it helps I'll be glad to tag and push a new version. I'm not sure I understood your remark regarding the major version number, do you have a reason why you would like to change it? As for the API, I think I wrote somewhere that things were still susceptible to break. Which means I don't have a problem at the moment with changing the API - for returning an error for example instead of panicking, I'm sure that's actually a good thing. What I said regarding the custom verifier is not that I don't want to change things, but that I am reluctant to change the @badboy Thanks! I'll try to see if I can make it work with real errors before pushing. |
Follow-up on the verifier changes to prevent panic!() on verification errors. Here we change the String-s returned in previous commit to Error-s of type "Other", with an inner description. To be squashed with previous commit before merging.
|
So I added a commit to change strings into error, but feedback would be welcome again. I'm unsure this is the way I should do it, putting formatted description such as eBPF instruction number in the error description might not be the best thing to do. (Once it's ready I'll squash the commit with the previous one before merging) |
We just updated the behaviour of the `new()` and `set_program()` methods to build eBPF VM and change their programs, so that they return a `Result()` possibly containing an error. Programs trying to access the newly created VMs must now `unwrap()` or do something equivalent. Update the README.md file in that regard.
We changed the behaviour of the verifier to prevent him from panicking on errors, making him propagate errors instead. Remove the comments in the documentation of the eBPF VMs about the verifier being susceptible to panic!(). This will likely be squashed before being merged to master branch.
Submitting as a PR, because feedback from people better than myself at Rust would be welcome before I push :).
These two commits make the verifier return a
Result<(), String>instead of panicking when a program does not pass verification tests. This is in order to avoid exiting the (Rust) program automatically when a (eBPF) program is rejected.First commit wraps the call to
panic!()for program rejection in a single function, which also adds the"[Verifier] Error: "prefix to the messages. Then the second commit changes thispanic!()into an error that can be propagated upwards, and updates all tests withunwrap()each time this is fit. All tests pass (yeah!).To do: This changes the basic API, the README file and the example invocation it contains will really need an update after we merge this.
Addresses, at least in parts, issue #30.