Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rust: support rich errors #769

Merged
merged 5 commits into from
Apr 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bindings/rust/integration-test/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@
extern crate fizzy;

fn main() {
assert_eq!(fizzy::validate(&[]), false);
assert!(fizzy::validate(&[]).is_ok());
println!("Fizzy works!");
}
116 changes: 95 additions & 21 deletions bindings/rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,69 @@

mod sys;

use std::ffi::CString;
use std::ffi::{CStr, CString};
use std::ptr::NonNull;

/// A safe container for handling the low-level FizzyError struct.
struct FizzyErrorBox(Box<sys::FizzyError>);

impl FizzyErrorBox {
/// Create a safe, boxed, and zero initialised container.
fn new() -> Self {
FizzyErrorBox {
0: Box::new(sys::FizzyError {
code: 0,
message: [0i8; 256],
}),
}
}

/// Return a pointer passable to low-level functions (validate, parse, instantiate).
///
/// # Safety
/// The returned pointer is still onwed by this struct, and thus not valid after this struct goes out of scope.
unsafe fn as_mut_ptr(&mut self) -> *mut sys::FizzyError {
&mut *self.0
}

/// Return the underlying error code.
// TODO: represent the errors as proper Rust enums
fn code(&self) -> u32 {
self.0.code
}

/// Return an owned String copy of the underlying message.
fn message(&self) -> String {
unsafe {
CStr::from_ptr(self.0.message.as_ptr())
.to_string_lossy()
.into_owned()
}
}
}

impl std::fmt::Display for FizzyErrorBox {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "{} [{}]", self.code(), self.message())
}
}

/// Parse and validate the input according to WebAssembly 1.0 rules. Returns true if the supplied input is valid.
pub fn validate<T: AsRef<[u8]>>(input: T) -> bool {
unsafe {
pub fn validate<T: AsRef<[u8]>>(input: T) -> Result<(), String> {
let mut err = FizzyErrorBox::new();
let ret = unsafe {
sys::fizzy_validate(
input.as_ref().as_ptr(),
input.as_ref().len(),
std::ptr::null_mut(),
err.as_mut_ptr(),
)
};
if ret {
debug_assert!(err.code() == 0);
Copy link
Member Author

Choose a reason for hiding this comment

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

These assertions show the current error reporting system is not prefect, as there are two ways to tell failure, error code or the return value.

Copy link
Collaborator

@gumb0 gumb0 Mar 31, 2021

Choose a reason for hiding this comment

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

For validate theoretically we could return error code instead of bool (plus optional error message), but for parse and instantiate it seems inevitable: we have to return a pointer to module / instance in any case, and when we also return an error code this pointer is going to be NULL.

Copy link
Member Author

@axic axic Mar 31, 2021

Choose a reason for hiding this comment

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

Do we want to add assertions into the capi for this? Or at least extend the tests? And document it?

These debug_asserts here act like the C assert, they are not part of a release build. Happy to keep them though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to add assertions into the capi for this? Or at least extend the tests? And document it?

Tests already have checks for both return value and error for both failure and success cases.

Assertions in C API - not sure where and which ones (it seems kind of obvious there that error is set when false or nullptr is returned)

Document it - something likeReturned error code is FIZZY_SUCCESS iff return value is true? Sounds somewhat obvious, too...

Copy link
Member Author

@axic axic Apr 8, 2021

Choose a reason for hiding this comment

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

I guess documentation is fine just saying that error code must be FIZZY_SUCCESS if a pointer is returned and must NOT be FIZZY_SUCCESS otherwise? Basically we want to give indication to the user that they do not need to check both.

Ok(())
} else {
debug_assert!(err.code() != 0);
Err(err.message())
}
}

Expand All @@ -74,17 +126,21 @@ impl Clone for Module {

/// Parse and validate the input according to WebAssembly 1.0 rules.
pub fn parse<T: AsRef<[u8]>>(input: &T) -> Result<Module, String> {
let mut err = FizzyErrorBox::new();
let ptr = unsafe {
sys::fizzy_parse(
input.as_ref().as_ptr(),
input.as_ref().len(),
std::ptr::null_mut(),
err.as_mut_ptr(),
)
};
if ptr.is_null() {
return Err("parsing failure".to_string());
debug_assert!(err.code() != 0);
Err(err.message())
} else {
debug_assert!(err.code() == 0);
Ok(Module { 0: ptr })
}
Ok(Module { 0: ptr })
}

/// An instance of a module.
Expand All @@ -101,6 +157,7 @@ impl Module {
// TODO: support imported functions
pub fn instantiate(self) -> Result<Instance, String> {
debug_assert!(!self.0.is_null());
let mut err = FizzyErrorBox::new();
let ptr = unsafe {
sys::fizzy_instantiate(
self.0,
Expand All @@ -110,17 +167,20 @@ impl Module {
std::ptr::null(),
std::ptr::null(),
0,
std::ptr::null_mut(),
err.as_mut_ptr(),
)
};
// Forget Module (and avoid calling drop) because it has been consumed by instantiate (even if it failed).
core::mem::forget(self);
if ptr.is_null() {
return Err("instantiation failure".to_string());
debug_assert!(err.code() != 0);
Err(err.message())
} else {
debug_assert!(err.code() == 0);
Ok(Instance {
0: unsafe { NonNull::new_unchecked(ptr) },
})
}
Ok(Instance {
0: unsafe { NonNull::new_unchecked(ptr) },
})
}
}

Expand Down Expand Up @@ -453,6 +513,15 @@ impl Instance {
mod tests {
use super::*;

#[test]
fn error_box() {
let mut err = FizzyErrorBox::new();
assert_ne!(unsafe { err.as_mut_ptr() }, std::ptr::null_mut());
assert_eq!(err.code(), 0);
assert_eq!(err.message(), "");
assert_eq!(format!("{}", err), "0 []");
}

#[test]
fn value_conversions() {
// NOTE: since the underlying type is a union, a conversion or access to other members is undefined
Expand Down Expand Up @@ -622,18 +691,20 @@ mod tests {
#[test]
fn validate_wasm() {
// Empty
assert_eq!(validate(&[]), false);
assert_eq!(validate(&[]).err().unwrap(), "invalid wasm module prefix");
// Too short
assert_eq!(validate(&[0x00]), false);
// Valid
assert_eq!(
validate(&[0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00]),
true
validate(&[0x00]).err().unwrap(),
"invalid wasm module prefix"
);
// Valid
assert!(validate(&[0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00]).is_ok());
// Invalid version
assert_eq!(
validate(&[0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x01]),
false
validate(&[0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x01])
.err()
.unwrap(),
"invalid wasm module prefix"
);
}

Expand All @@ -644,7 +715,7 @@ mod tests {
parse(&[0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x01])
.err()
.unwrap(),
"parsing failure"
"invalid wasm module prefix"
);
}

Expand All @@ -667,7 +738,10 @@ mod tests {
let module = parse(&input);
assert!(module.is_ok());
let instance = module.unwrap().instantiate();
assert_eq!(instance.err().unwrap(), "instantiation failure");
assert_eq!(
instance.err().unwrap(),
"module defines an imported memory but none was provided"
);
}

#[test]
Expand Down