miracl / amcl

33 stars 20 forks source link

invmodp in big goes in infinite loop when inverting 0 #34

Open lovesh opened 5 years ago

lovesh commented 5 years ago

I am using the Rust code. When using invmodp in big.rs here for inverting 0, it gets stuck in endless loop instead of returning 0. Or should it instead be an exception since we expect inverse to follow x*inverse(x)=1

webmaster128 commented 5 years ago

I can reproduce the behaviour in a JS test. For integers, this is typically a case for a division by zero exception, like e.g. in Python

>>> 1/0
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ZeroDivisionError: division by zero

However, as far as I can see, no exceptions are used in the library. This is probably because not all supported languages have exceptions. But is there a better way to report those kind of errors?

lovesh commented 5 years ago

Ideally, the code for every language should be idiomatic in that language. So the Rust code should raise an error while the C code should return a non-zero error code.

mcarrickscott commented 5 years ago

I guess just returning 0 is better than an infinite loop, while admittedly not mathematically correct. And yes I want to avoid language specific constructs if at all possible, otherwise maintenance becomes a nightmare.

webmaster128 commented 5 years ago

I think returning 0 hides a logic error of the caller code much better than an infinite loop, such that bugs might be undetected much longer. At the end of the day, the caller must not call invmodp on 0.

mcarrickscott commented 5 years ago

Fair point. But in the multiplicative group GF(p) the value 0 is actually not a group member, so returning an illegal value does flag up the problem if the caller is paying attention. Also the alternate constant time method for modular inversion that is actually used in the elliptic curve code uses Fermats method, 1/x = x^{p-2} mod p, which does naturally return 0 for x=0.

webmaster128 commented 5 years ago

so returning an illegal value does flag up the problem if the caller is paying attention

Nice one