alexcrichton opened issue #4439:
Currently the following test will fail:
let mut config = Config::new(); config.consume_fuel(true); let engine = Engine::new(&config).unwrap(); let mut store = Store::new(&engine, ()); store.add_fuel(u64::MAX).unwrap(); // fuel wraps to i64 so the store has i64::MAX fuel now assert_eq!(store.consume_fuel(0).unwrap(), i64::MAX as u64); // consume all but one fuel assert_eq!(store.consume_fuel(i64::MAX as u64 - 1).unwrap(), 1); // confirm that one fuel remains assert_eq!(store.consume_fuel(0).unwrap(), 1); // try to add fuel to get to 11 more fuel store.add_fuel(10).unwrap(); assert_eq!(store.consume_fuel(0).unwrap(), 11); // <-- this assertion will fail since the store says it has 1 fuel
The "bug" is here where when 10 fuel is added the counters there look like:
[crates/wasmtime/src/store.rs:1411] adj = 9223372036854775807 [crates/wasmtime/src/store.rs:1412] &consumed_ptr = -1 [crates/wasmtime/src/store.rs:1413] fuel = 10
where
adj
is alreadyi64::MAX
soadj.checked_add(fuel)
overflows and we hit the overflow case which resets everything back to its prior value.Unfortunately I don't think that this is fixable with the current APIs of
Store
. CurrentlyStore::consumed_fuel
will report how much fuel has been consumed for the entire lifetime of the store which requires keeping thefuel_adj
counter around, meaning that we can't actually represent consumingu64::MAX
fuel since thefuel_adj
is ani64
.I don't know how to best fix this myself, but we have some fuzz targets using this pattern and suffering from it so we'll need to fix them.
alexcrichton labeled issue #4439:
Currently the following test will fail:
let mut config = Config::new(); config.consume_fuel(true); let engine = Engine::new(&config).unwrap(); let mut store = Store::new(&engine, ()); store.add_fuel(u64::MAX).unwrap(); // fuel wraps to i64 so the store has i64::MAX fuel now assert_eq!(store.consume_fuel(0).unwrap(), i64::MAX as u64); // consume all but one fuel assert_eq!(store.consume_fuel(i64::MAX as u64 - 1).unwrap(), 1); // confirm that one fuel remains assert_eq!(store.consume_fuel(0).unwrap(), 1); // try to add fuel to get to 11 more fuel store.add_fuel(10).unwrap(); assert_eq!(store.consume_fuel(0).unwrap(), 11); // <-- this assertion will fail since the store says it has 1 fuel
The "bug" is here where when 10 fuel is added the counters there look like:
[crates/wasmtime/src/store.rs:1411] adj = 9223372036854775807 [crates/wasmtime/src/store.rs:1412] &consumed_ptr = -1 [crates/wasmtime/src/store.rs:1413] fuel = 10
where
adj
is alreadyi64::MAX
soadj.checked_add(fuel)
overflows and we hit the overflow case which resets everything back to its prior value.Unfortunately I don't think that this is fixable with the current APIs of
Store
. CurrentlyStore::consumed_fuel
will report how much fuel has been consumed for the entire lifetime of the store which requires keeping thefuel_adj
counter around, meaning that we can't actually represent consumingu64::MAX
fuel since thefuel_adj
is ani64
.I don't know how to best fix this myself, but we have some fuzz targets using this pattern and suffering from it so we'll need to fix them.
github-actions[bot] commented on issue #4439:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
fitzgen commented on issue #4439:
Rather than resetting on overflow, what if we set to
MAX - 1
? or justMAX
itself?
alexcrichton commented on issue #4439:
The main problem is the
Store::consumed_fuel
API. We can reset counters but then that function isn't correct any more since it's moreso fuel consumed since the last call toadd_fuel
. That's perhaps not the worst interpretation of the API and would indeed allow fixing this issue.
alexcrichton closed issue #4439:
Currently the following test will fail:
let mut config = Config::new(); config.consume_fuel(true); let engine = Engine::new(&config).unwrap(); let mut store = Store::new(&engine, ()); store.add_fuel(u64::MAX).unwrap(); // fuel wraps to i64 so the store has i64::MAX fuel now assert_eq!(store.consume_fuel(0).unwrap(), i64::MAX as u64); // consume all but one fuel assert_eq!(store.consume_fuel(i64::MAX as u64 - 1).unwrap(), 1); // confirm that one fuel remains assert_eq!(store.consume_fuel(0).unwrap(), 1); // try to add fuel to get to 11 more fuel store.add_fuel(10).unwrap(); assert_eq!(store.consume_fuel(0).unwrap(), 11); // <-- this assertion will fail since the store says it has 1 fuel
The "bug" is here where when 10 fuel is added the counters there look like:
[crates/wasmtime/src/store.rs:1411] adj = 9223372036854775807 [crates/wasmtime/src/store.rs:1412] &consumed_ptr = -1 [crates/wasmtime/src/store.rs:1413] fuel = 10
where
adj
is alreadyi64::MAX
soadj.checked_add(fuel)
overflows and we hit the overflow case which resets everything back to its prior value.Unfortunately I don't think that this is fixable with the current APIs of
Store
. CurrentlyStore::consumed_fuel
will report how much fuel has been consumed for the entire lifetime of the store which requires keeping thefuel_adj
counter around, meaning that we can't actually represent consumingu64::MAX
fuel since thefuel_adj
is ani64
.I don't know how to best fix this myself, but we have some fuzz targets using this pattern and suffering from it so we'll need to fix them.
alexcrichton commented on issue #4439:
I believe this is now fixed now that
consume_fuel
is gone.
Last updated: Dec 23 2024 at 12:05 UTC