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

fix(c-api) Call wasi_env_delete manually #2090

Merged
merged 3 commits into from
Feb 2, 2021

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Feb 2, 2021

Description

wasi_get_imports isn't taking ownership of wasi_env_t (despites
what is written in the documentation). And it must not take ownership
of it, since one could use it with the wasi_env_read_stdout &
sibling functions after having called wasi_get_imports.

Consequently, this patch calls wasi_env_delete to avoid leaking
memory.

Review

  • Add a short description of the the change to the CHANGELOG.md file

`wasi_get_imports` isn't taking ownership of `wasi_env_t` (despites
what is written in the documentation). And it must not take ownership
of it, since one could use it with the `wasi_env_read_stdout` &
sibling functions after having called `wasi_get_imports`.

Consequently, this patch calls `wasi_env_delete` to avoid leaking
memory.
@Hywan Hywan added bug Something isn't working 📦 lib-c-api About wasmer-c-api labels Feb 2, 2021
@Hywan Hywan self-assigned this Feb 2, 2021
@Hywan Hywan requested a review from syrusakbary as a code owner February 2, 2021 13:30
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Posted on Slack, but I think this is not symmetric with what we do in Rust and that the wasi_env_t is invalid so it's a bit odd to require manually deleting it in my opinion

@Hywan
Copy link
Contributor Author

Hywan commented Feb 2, 2021

We don't take ownership of wasi_env_t. Just a reference:

#[no_mangle]
pub unsafe extern "C" fn wasi_get_imports(
store: Option<&wasm_store_t>,
module: Option<&wasm_module_t>,
wasi_env: Option<&wasi_env_t>,
imports: &mut wasm_extern_vec_t,
) -> bool {

And we just clone the inner value:

let import_object = generate_import_object_from_env(store, wasi_env.inner.clone(), version);

So WasiEnv is still available and fully working :-).

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Alright, if it works then maybe that's a feature. The docs on the wasi_env_t type itself might need to be updated, I suspect they probably say that it does not need to be deleted.

@Hywan
Copy link
Contributor Author

Hywan commented Feb 2, 2021

Documentation has already been updated for wasi_get_imports and wasi_get_unordered_imports.

@Hywan
Copy link
Contributor Author

Hywan commented Feb 2, 2021

bors r+

bors bot added a commit that referenced this pull request Feb 2, 2021
2090: fix(c-api) Call `wasi_env_delete` manually r=Hywan a=Hywan

# Description

`wasi_get_imports` isn't taking ownership of `wasi_env_t` (despites
what is written in the documentation). And it must not take ownership
of it, since one could use it with the `wasi_env_read_stdout` &
sibling functions after having called `wasi_get_imports`.

Consequently, this patch calls `wasi_env_delete` to avoid leaking
memory.

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Ivan Enderlin <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 2, 2021

Build failed:

@Hywan
Copy link
Contributor Author

Hywan commented Feb 2, 2021

I believe these is something bigger happening here. cc @MarkMcCaskey

@Hywan
Copy link
Contributor Author

Hywan commented Feb 2, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 2, 2021

@bors bors bot merged commit 820ffe9 into wasmerio:master Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📦 lib-c-api About wasmer-c-api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants