checkpoint: confirm affect in pdk with offset check

[nilslice]

@zshipko - I've updated and tested the pdk such that config (and Variables.get for that matter) do an offset check before getting the lengt

 config(key: string): string {
    const mem = this.allocateString(key)

    const offset = extism_config_get(mem.offset)
    if (offset == 0) {
      return ""
    }

    const length = extism_length(offset)
    if (length == 0) {
      return ""
    }

    let buffer: ArrayBuffer = new ArrayBuffer(u32(length));
    let value: Uint8Array = Uint8Array.wrap(buffer)
    load(offset, value)
    return String.UTF8.decode(buffer)
  }

I don't see any difference from this change after reproducing the error, which is: extism::sdk ERROR 2022-09-09T14:13:50.285141-06:00 (runtime/src/sdk.rs:162) - Call: Invalid config key

I will take a look in runtime/src/sdk.rs, but wanted to keep you on the same page in case you had any ideas. Thanks!

[nilslice]

@zshipko - Ok to test this now, I've added some updates across the board:

1. extism/extism checkout (which fixes this PR's issue)
2. extism CLI: https://github.com/extism/cli/pull/5
3. rebuild the example.wasm (make example)
4. extism call example.wasm --input "this is a test" --set-config='{"thing": "1234"}' count_vowels

Note that the example has updated to expect a string. If I try using a number in the --set-config "thing" key, no value is returned (even after trying to convert the value to string in example. not sure what's up there.)

[zshipko]

Yeah, I can confirm that this doesn't fix the problem, with this PR https://github.com/extism/extism/pull/9 it seems to be working.

[zshipko]

I also think get/config should return null when no value is found instead of an empty string. I can push that change if you agree.

[nilslice]

I also think get/config should return null when no value is found instead of an empty string. I can push that change if you agree.

Yea, definitely - thank you!

[zshipko]

Cool, just pushed that. Once you check this against extism it should be ready to go!

[nilslice]

@zshipko nice, works great! I will update the test to check for null. This is still bugging me though.. I think it's a asc specific thing, but haven't tried another PDK yet:

extism call example.wasm --input "this is a test" --set-config='{"thing": 1}' count_vowels

returns {"count": 4, "config": "null"}, but would expect {"count": 4, "config": 1}

Any ideas?

[zshipko]

It looks like when we convert from the JSON string using serde it expects a string but instead of raising an error it just silently ignores the non-string values.

Right now we're just using BTreeMap<String, String> for config but we could probably switch to BTreeMap<String, serde_json::Value> to fix this.

[nilslice]

Right now we're just using BTreeMap<String, String> for config but we could probably switch to BTreeMap<String, serde_json::Value> to fix this.

Ah, good call. I'm about to leave for a bit, but can test this out when I'm home.

[nilslice]

Ok.. not sure the juice is worth the squeeze here, especially just for config values. There's a lot of type matching necessary to go from serde_json::Value -> raw bytes, unless we convert everything to strings first. And then in PDK side, especially this one, going beyond the current return type string | null is not clear to me, unless the decoding is done by the user after the value is returned as raw bytes...