libtom / libtommath

LibTomMath is a free open source portable number theoretic multiple-precision integer library written entirely in C.
https://www.libtom.net
Other
650 stars 194 forks source link

Fix possible integer overflow #555

Closed sjaeckel closed 1 year ago

sjaeckel commented 1 year ago

This is #546 but on develop.

sjaeckel commented 1 year ago

Wat?

https://github.com/libtom/libtommath/actions/runs/5144260217/jobs/9260336791?pr=555#step:4:333

demo/test.c:1112:13: runtime error: negation of -2147483648 cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself

Do I understand this correctly that s_mp_rand_jenkins() created the only value that can't be converted with abs()? :D

czurnieden commented 1 year ago

Do I understand this correctly that s_mp_rand_jenkins() created the only value that can't be converted with abs()?

Yes, it is a good boy^WPRNG. Only bad for us ;-)

Found it at three places in test.c , so it isn't that bad. Do you want to put a note in the documentation/github-wiki for the developers-to-come?

sjaeckel commented 1 year ago

I just thought whether it'd be good to add runtime checks&fixups to the rand_xx() functions like if (x == xx_MIN) x++;!?

czurnieden commented 1 year ago

I just thought whether it'd be good to add runtime checks&fixups to the rand_xx() functions like if (x == xx_MIN) x++;!?

Changing the output of a PRNG is always *hng* for me but is probably the easiest.

How about offering unsigned variations of rand_xx()? Would make abs() obsolete.

sjaeckel commented 1 year ago

How about offering unsigned variations of rand_xx()? Would make abs() obsolete.

absolutely!

czurnieden commented 1 year ago

absolutely!

Alhaalta ylös! ;-)

Copy&paste a check into every single rand_xx or C&P the whole stick and C&P some unsigneds and us in front of the types? But to be honest: it's the test environment, it must be correct, not shiny! Whatever is less work: I'll let it through on the nod.

sjaeckel commented 1 year ago

In a next step we should follow up on #430 ff.