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) Memory leak around wasm_$name_vec_delete #1853

Closed
wants to merge 2 commits into from

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Dec 1, 2020

Description

WIP

Review

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

Hywan added 2 commits December 1, 2020 15:30
This patch does several things:

1. It applies our Rust patterns for C API by replacing the raw pointer
   by `Option<Box<T>>`,
2. It allows `wasm_$name_vec_delete` to handle null pointer,
3. Because it takes ownership of the `wasm_$name_vec_t`, the pointer
   is correctly dropped (which fix the memory leak).
@Hywan Hywan added bug Something isn't working 📦 lib-c-api About wasmer-c-api labels Dec 1, 2020
@Hywan Hywan requested a review from MarkMcCaskey December 1, 2020 14:31
@Hywan Hywan self-assigned this Dec 1, 2020
let _ = Vec::from_raw_parts(subject.data, subject.size, subject.size);
}

::std::ptr::drop_in_place(Box::into_raw(subject));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without that, everything compiles nicely but we have a free error when executing a C program.

bors bot added a commit that referenced this pull request Dec 1, 2020
1855: Fix memory leak in wat2wasm function r=MarkMcCaskey a=MarkMcCaskey

Alternative to #1853 ; 

Found with @nlewycky 

# Review

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


Co-authored-by: Mark McCaskey <mark@wasmer.io>
Co-authored-by: Mark McCaskey <5770194+MarkMcCaskey@users.noreply.github.com>
@jubianchi
Copy link
Contributor

Closing this one as #1855 got merged and is an alternative to this patch.

@jubianchi jubianchi closed this Dec 2, 2020
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