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

Multiplication of 32 bit ints is not following mod 2^32 bits, like does in C #41

Closed
3nsoft opened this issue Jul 22, 2014 · 7 comments
Closed

Comments

@3nsoft
Copy link

3nsoft commented Jul 22, 2014

In C, when int32 is multiplied with int32, overflowing, operation works as multiplication mod 2^32 bits. I.e. in C a_b === a_b mod 2^32.

Do run https://github.com/3nsoft/ecma-nacl/blob/master/tests/util/int32.js in node. It shows that in JS, for some values a_b === a_b mod 2^32, and a_b !== a_b mod 2^32 for others.
It has to do with JS treating numbers the same way, irrespective of whether they come from Uint32Array, or not, -- all are treated as JS numbers (60 bits + funky stuff?).

I can see that your code simply uses multiplication. It might be not very good.

Also, add tests, which will take huge amount of random inputs and go through your code and https://github.com/tonyg/js-nacl which would be the fastest way to reproduces strict functional comparison with original C code.
I'd love to have a test vector, which will make current code fail, but I only have a theoretical reason that it may fail. Yet, I see no way to prove, that it won't fail.

@dchest
Copy link
Owner

dchest commented Jul 22, 2014

Thanks, I was browsing your code an hour ago and noticed the same. I'm checking all uses of multiplications in TweetNaCl-js now.

FYI, we have tests that compare random inputs with tweetnacl.c: https://github.com/dchest/tweetnacl-js/tree/master/test/c

@3nsoft
Copy link
Author

3nsoft commented Jul 22, 2014

Notice that it is OK to multiply by constants like 24, and have an addition operation before smashing it back into Uint32. But for readability sake, I am thinking to change all multiplication to proper ones. Less confusion.

I'd love you to do cross-tests + benchmarks with js-nacl. You may adapt ecma-nacl's tests. Add ecma into comparison. Let's see a benchmark war!!

Kudos.

@3nsoft
Copy link
Author

3nsoft commented Jul 22, 2014

Can you flip comparison from tweetnacl to original nacl.
People suggest running original in production environment. Ya know, for the paranoid among us. Especially in light of your own disclaimer about code being not wetted.

@dchest
Copy link
Owner

dchest commented Jul 22, 2014

TweetNaCl is a production-ready library -- see http://tweetnacl.cr.yp.to/tweetnacl-20131229.pdf.

(I specifically wanted to port TweetNaCl, not the original NaCl due to its size, and also to keep visual differences at minimum -- see an outdated diff here).

@dchest
Copy link
Owner

dchest commented Jul 27, 2014

OK, in Poly1305, the only place that could be affected by this, we have this multiplication:

      for (j = 0; j < 17; j++) x[i] = (x[i] + (h[j] * ((j <= i) ? r[i - j] : ((320 * r[i + 17 - j])|0))) | 0) | 0;

Both h and r are maximum 255 (h comes from add1305 and r from k), so the maximum value we can have is 255 * (320 * 255) = 20808000. No overflowing here.

Objections before I close this?

@3nsoft
Copy link
Author

3nsoft commented Jul 27, 2014

Poly is cool.

@dchest dchest closed this as completed Jul 27, 2014
@dchest
Copy link
Owner

dchest commented Jul 27, 2014

Indeed.

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

No branches or pull requests

1 participant