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

Remove wee_alloc. #22

Merged
merged 1 commit into from
Feb 17, 2023
Merged

Conversation

AntoniosBarotsis
Copy link
Contributor

wee_alloc is no longer maintained and has a few critical open issues (memory leaks), see here.

$ cargo audit
    Fetching advisory database from `https://github.com/RustSec/advisory-db.git`
      Loaded 511 security advisories (from C:\Users\anton\.cargo\advisory-db)
    Updating crates.io index
    Scanning Cargo.lock for vulnerabilities (121 crate dependencies)
Crate:     wee_alloc
Version:   0.4.5
Warning:   unmaintained
Title:     wee_alloc is Unmaintained
Date:      2022-05-11
ID:        RUSTSEC-2022-0054
URL:       https://rustsec.org/advisories/RUSTSEC-2022-0054
Dependency tree:
wee_alloc 0.4.5
└── fast_qr 0.8.4

This reverts back to the recommended dlmalloc-rs.

I ran the wasm-pack.sh file and everything seems fine but I have not tested this from JS directly. A few other PRs seem to just remove the extra wee_alloc stuff with no other change so this should be fine. I have also not committed any of the changes that happen to the pkg/* files when I run wasm-pack.sh, should I do that?

@erwanvivien
Copy link
Owner

It is fine not to commit the pkg, I do that when upgrading version usually.

I will look at that, since it really helps reducing the size of the wasm package, thanks for the hindsight

@AntoniosBarotsis
Copy link
Contributor Author

There's also lol_alloc but the author mentioned how it's not really tested in production.

@erwanvivien
Copy link
Owner

I think we'll just go with no alloc, thus using dlmalloc-rs. Thanks Antonios

After trying, it "only" adds 2kB to the gzip archive, while it matters, it's better than leaking memory

@erwanvivien erwanvivien merged commit 700d38e into erwanvivien:master Feb 17, 2023
@maciejhirsz
Copy link

Hey @erwanvivien, can we get this patch published on crates.io? I've a project that uses fast_qr in Wasm, and GitHub's security bot keeps giving me grief 🙃.

@erwanvivien
Copy link
Owner

Dang, super sorry, I really thought I had done it. Will do right now

@erwanvivien
Copy link
Owner

@maciejhirsz Update to 8.5 :) Again, sorry for the delay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants