Skip to content

Commit

Permalink
Merge #1690
Browse files Browse the repository at this point in the history
1690: fix(c-api) `wasm_limits_t` contains `Pages`, not `Bytes` + sentinel value r=Hywan a=Hywan

When building a `wasm_memorytype_t` with `wasm_memorytype_new`, we
pass a `wasm_limits_t`, where `min` and `max` represent `Pages`. This
semantics is set by `wasm_memorytype_new` itself where `min` and `max`
from `wasm_limits_t` are used to compute `Pages`, which are then passed
to `MemoryType`.

Then, in `wasm_memorytype_limits`, we expect to get the same
`wasm_limits_t` given to `wasm_memorytype_new`. But it's not!

The same `MemoryType` is read, good. The `minimum` and `maximum`
fields are `Pages`, good. Then, we compute the `min` and `max` values
for the resulting `wasm_limits_t`, which receive `Page.bytes().0`, not
good! We don't want the number of bytes, but the number of pages.

This patch fixes that.

Edit: This patch also fixes the max limit sentinel value. In `wasm.h`, it is defined by `wasm_limits_max_default` (`0xffffffff`). In our code, it is correctly used in `wasm_memorytype_new`, but not in `wasm_memorytype_limits`. A new constant has been introduced: `LIMITS_MAX_SENTINEL`. Not super happy with this name though.

Co-authored-by: Ivan Enderlin <[email protected]>
  • Loading branch information
bors[bot] and Hywan authored Oct 8, 2020
2 parents 70baded + 3e77b8f commit 2459d5d
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Changelog

## **[Unreleased]**
- [#1690](https://github.com/wasmerio/wasmer/pull/1690) Fix `wasm_memorytype_limits` where `min` and `max` represents pages, not bytes. Additionally, fixes the max limit sentinel value.
- [#1635](https://github.com/wasmerio/wasmer/pull/1635) Implement `wat2wasm` in the Wasm C API.
- [#1636](https://github.com/wasmerio/wasmer/pull/1636) Implement `wasm_module_validate` in the Wasm C API.
- [#1671](https://github.com/wasmerio/wasmer/pull/1671) Fix probestack firing inappropriately, and sometimes over/under allocating stack.
Expand Down
13 changes: 10 additions & 3 deletions lib/c-api/src/wasm_c_api/types/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,18 @@ pub struct wasm_limits_t {
pub(crate) max: u32,
}

const LIMITS_MAX_SENTINEL: u32 = u32::max_value();

#[no_mangle]
pub unsafe extern "C" fn wasm_memorytype_new(limits: &wasm_limits_t) -> Box<wasm_memorytype_t> {
let min_pages = Pages(limits.min as _);
// u32::max_value() is a sentinel value for no max specified
let max_pages = if limits.max == u32::max_value() {
let max_pages = if limits.max == LIMITS_MAX_SENTINEL {
None
} else {
Some(Pages(limits.max as _))
};

Box::new(wasm_memorytype_t {
extern_: wasm_externtype_t {
inner: ExternType::Memory(MemoryType::new(min_pages, max_pages, false)),
Expand All @@ -54,8 +57,12 @@ pub unsafe extern "C" fn wasm_memorytype_delete(_memorytype: Option<Box<wasm_mem
#[no_mangle]
pub unsafe extern "C" fn wasm_memorytype_limits(mt: &wasm_memorytype_t) -> *const wasm_limits_t {
let md = mt.as_memorytype();

Box::into_raw(Box::new(wasm_limits_t {
min: md.minimum.bytes().0 as _,
max: md.maximum.map(|max| max.bytes().0 as _).unwrap_or(0),
min: md.minimum.0 as _,
max: md
.maximum
.map(|max| max.0 as _)
.unwrap_or(LIMITS_MAX_SENTINEL),
}))
}

0 comments on commit 2459d5d

Please sign in to comment.