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

createHash memory leak #786

Closed
shamblesides opened this issue Mar 5, 2021 · 7 comments · Fixed by #1025
Closed

createHash memory leak #786

shamblesides opened this issue Mar 5, 2021 · 7 comments · Fixed by #1025

Comments

@shamblesides
Copy link

shamblesides commented Mar 5, 2021

Discovered this because pbkdf2 from node/crypto was leaking memory, but the hash module seems to be the source. This script increases in memory at a rate of 50MB/s on my machine, indefinitely (I stopped it at 1GB):

import { createHash } from "https://deno.land/std@0.89.0/hash/mod.ts"

while (1) {
  createHash('sha512')
}

It appears to happen with any hash, not just sha512.

Verison info

$ deno --version
deno 1.8.0 (release, x86_64-unknown-linux-gnu)
v8 9.0.257.3
typescript 4.2.2
@nettybun
Copy link

nettybun commented Mar 6, 2021

I think it's because it's instantiated in WASM rather than in JS.
https://www.github.com/denoland/deno_std/tree/main/hash%2F_wasm%2Fwasm.js

It looks like you can call free() manually which should help.

@shamblesides
Copy link
Author

shamblesides commented Mar 6, 2021

For my particular use case, I guess that means that deno_std will need to be a PR that adds the free() call somewhere around here: https://github.com/denoland/deno_std/blob/main/node/_crypto/pbkdf2.ts#L33

@nettybun
Copy link

nettybun commented Mar 6, 2021

Hmm well createHasher returns an anonymous function, so you could add free as a property to the function so it would be available to manually call.

@shamblesides
Copy link
Author

shamblesides commented Mar 6, 2021

Actually it looks like this won't work without changes to the hash module itself.

The public module function, createHash:

https://github.com/denoland/deno_std/blob/main/hash/mod.ts#L34

returns a Hash object:

https://github.com/denoland/deno_std/blob/main/hash/_wasm/hash.ts#L19

and the Hash object has a private property called #hash which is populated by calling a (completely different) function called createHash which is generated by wasm:

https://github.com/denoland/deno_std/blob/main/hash/_wasm/hash.ts#L24

so if I want to call free() I have to access private properties on a class defined within the _wasm folder, which seems like a hack to me. I don't think this is the way this module was intended to be used.

If it is indeed necessary that I must manually free() the hash object, then it should definitely be documented here: https://deno.land/std@0.89.0/hash And, the node/crypto module needs to be patched. Otherwise, it seems like some changes need to be made in the rust code, and I'm afraid I don't know enough about WASM and Rust to be able to do that.

@caspervonb
Copy link
Contributor

digest from crypto.subtle should superceede this by 1.9

@shamblesides
Copy link
Author

Sounds good, then I will wait for crypto.subtle

Do you think deriveKey will also be available? That's the interface I'm really wanting to use, for PBKDF2

@caspervonb
Copy link
Contributor

caspervonb commented Mar 6, 2021

Maybe, we'll see how far we get :)

denoland/deno#9614

wperron added a commit that referenced this issue Jul 7, 2021
wasm-pack limits our ability to configure wasm-bindgen and is no
longer necessary as we can use wasm-bindgen-cli directly. This
allows us to enable --weakrefs which partially mitigates #786 by
using a FinalizationRegistry (currently working in Deno Canary,
though not yet on Stable) to free memory in the WASM heap
automatically when the corresponding JavaScript wrapper objects
are garbage-collected.

Co-authored-by: William Perron <hey@wperron.io>
@ry ry closed this as completed in #1025 Jul 29, 2021
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 a pull request may close this issue.

3 participants