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

Potential hash collision by using v8::Module::GetIdentityHash() #518

Closed
ry opened this issue Feb 11, 2019 · 9 comments
Closed

Potential hash collision by using v8::Module::GetIdentityHash() #518

ry opened this issue Feb 11, 2019 · 9 comments
Labels
wontfix This will not be worked on

Comments

@ry
Copy link
Member

ry commented Feb 11, 2019

https://github.com/denoland/deno/blob/1e5e091cb074896c7550b1b6f802582f12629048/libdeno/binding.cc#L293

Seems very unlikely people will hit this issue - but creating something just to track it.

@harthur
Copy link

harthur commented Apr 2, 2019

I came across this when searching for other projects having issues with GetIdentityHash. Circling back around just to say that I found out it's only 21 bits now, so we in another project were having issues with collisions in just hundreds of instances.

@ry
Copy link
Member Author

ry commented Apr 3, 2019

Ok thanks - good to know.

@zbendefy
Copy link

Hi! Is there an estimate on this issue? We're using GetIdentityHash() to identify objects, and we would like to know if we can rely on it or not.

@ry
Copy link
Member Author

ry commented Jun 25, 2019

@zbendefy I've had no problems with it.

@zbendefy
Copy link

@ry We'll use potentially thousands of objects, that we are identifying (for storage in a hashmap) using GetIdentityHash(). We didn't do thorough testing yet, but having a Stable way to identify JS objects would be nice.

I presume that we cannot use the underlying pointer values behind a Local<> or Persistent<> handle?

@lucacasonato
Copy link
Member

@ry Is this still relevant?

@ry
Copy link
Member Author

ry commented Aug 8, 2020

Yes

@kitsonk kitsonk transferred this issue from denoland/deno Nov 5, 2020
@kitsonk
Copy link

kitsonk commented Nov 5, 2020

These bindings are now in rust_v8 so I moved it.

@bnoordhuis
Copy link
Contributor

This is not for rusty_v8 to fix. The identity hash itself isn't buggy but it can be used in a way that introduces bugs.

To summarize:

  1. An object's identity hash is a 21 or 22 bits random number (on 32 and 64 bits architectures respectively)

  2. Per the Birthday Paradox, that means the chance of a collision is > 50% with approx. 1700 or 2400 objects.

You can use it to store objects in a hashmap but you need to additionally check with a.same_value(b) if the object already exists in the hashmap bucket.

@bnoordhuis bnoordhuis added the wontfix This will not be worked on label Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

6 participants