wasmtime / issue #4439 Store::consume_fuel with u64::MAX ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #4439 Store::consume_fuel with u64::MAX ...

Wasmtime GitHub notifications bot (Jul 13 2022 at 14:33):

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 already i64::MAX so adj.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. Currently Store::consumed_fuel will report how much fuel has been consumed for the entire lifetime of the store which requires keeping the fuel_adj counter around, meaning that we can't actually represent consuming u64::MAX fuel since the fuel_adj is an i64.

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.

Wasmtime GitHub notifications bot (Jul 13 2022 at 14:33):

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 already i64::MAX so adj.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. Currently Store::consumed_fuel will report how much fuel has been consumed for the entire lifetime of the store which requires keeping the fuel_adj counter around, meaning that we can't actually represent consuming u64::MAX fuel since the fuel_adj is an i64.

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.

Wasmtime GitHub notifications bot (Jul 13 2022 at 14:34):

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>

Wasmtime GitHub notifications bot (Jul 15 2022 at 19:39):

fitzgen commented on issue #4439:

Rather than resetting on overflow, what if we set to MAX - 1? or just MAX itself?

Wasmtime GitHub notifications bot (Jul 15 2022 at 19:55):

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 to add_fuel. That's perhaps not the worst interpretation of the API and would indeed allow fixing this issue.

Wasmtime GitHub notifications bot (Mar 20 2024 at 23:08):

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 already i64::MAX so adj.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. Currently Store::consumed_fuel will report how much fuel has been consumed for the entire lifetime of the store which requires keeping the fuel_adj counter around, meaning that we can't actually represent consuming u64::MAX fuel since the fuel_adj is an i64.

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.

Wasmtime GitHub notifications bot (Mar 20 2024 at 23:08):

alexcrichton commented on issue #4439:

I believe this is now fixed now that consume_fuel is gone.

Last updated: Oct 23 2024 at 20:03 UTC