JoeStrout / miniscript

source code of both C# and C++ implementations of the MiniScript scripting language
MIT License
280 stars 64 forks source link

Bitwise operations give invalid results with values > 2^31 #96

Closed Olipro closed 8 months ago

Olipro commented 1 year ago

These all return 0: bitAnd(4294967295, 1234) //0xFFFFFFFF -> expected: 1234 bitAnd(2147483655, 7) //0x80000007 -> expected: 7

bitOr isn't entirely immune either: bitOr(2147483648, 1234) gives -2147482414 which, is technically the right bits, but not actually because a number in Miniscript is a 64-bit double so the bits are all in the wrong place.

If all bitwise operations were achieved through first converting to a 64-bit uint. This should all become sane. Of course, if a user passes a double that's completely out of range of a uint64, that's on them. But that will be as broken then as it is now.

Olipro commented 1 year ago

I wanted to add some further context to this issue to explain exactly why using a 64-bit unsigned is the right thing to do, and some constraints you may wish to add to the bitops functions to ensure the right thing is done.

MiniScript numbers are a double - which on any contemporary system is going to be an IEEE-754 double-precision floating value. For the purposes of this post, I'm not going to consider esoteric architectures that might happen to run MiniScript, because all bets are off.

The max and min values of a double are far outside the range of even a 64-bit unsigned type, but, there is a valid contiguous range.

The happy case

a double can store every integral positive value from 0 to 2^53 inclusive - absolutely everything in this range can and should be converted into an unsigned 64 bit, the bitwise op performed and then converted back to a double. You will always get the right result back. these are defined in the C and C++ standard and every conforming implementation must do the same thing. C# also follows this.

The weird case

a double can also store negative values in the range -1 to -2^53 inclusive. The result of bitwise ops on negative values is declared to be implementation defined - though practically speaking, pretty much every architecture uses twos-complement so now we have to decide what to do.

Well, as I mentioned earlier, a unique property of double is that, unlike signed integral values, its valid range is identical both in the positive and negative space. So here is my proposal applicable to any bitop:

  1. Separate the sign bit (if any) from both operands - i.e. -12345 becomes 12345 and perform the bitop.
  2. Apply the bitop for the sign bit. (e.g. if it's a bitAnd, then the result will be negative only if both operands were negative to begin with.
  3. Apply the resulting sign bit (if any) back - i.e. return 0 - <result of step 1>

The undefined case

If either operand to a bitop is outside of 2^53 - -2^53 then all bets are off. The conversion to an integral could result in loss of bits, the conversion back could result in loss of bits. There is nothing meaningful that can be done here. Stick a warning in the documentation and call it a day.

JoeStrout commented 1 year ago

Thank you for this thoughtful proposal! Well-considered and well written, too. At the moment I can find nothing to object to in it.