Fix overflow when reading negative s8

[cherue]

When doing bitwise operations in JS the values are always converted to a signed (edit: not always signed, see here) 32-bit integer and the result is also always a signed 32-bit integer.

The calculation that converts two u4s to one s8 assumes only positive inputs, this change makes sure of that.

I discovered this when writing a test for JS 53-bit integer overflows, but that test and the error on overflow aren't part of this.

[GreyCat]

@cherue, can you clarify which test shows the problem in JS right now?

[cherue]

Right now no test shows the problem.

I am writing a test called js_overflow that will test that the JS runtime throws an error when reading numbers smaller than Number.MIN_SAFE_INTEGER or bigger than Number.MAX_SAFE_INTEGER (this is not implemented yet).

The ksy for this test currently looks like this:

meta:
  id: js_overflow
seq:
  - id: signed_negative_be
    type: s8be
  - id: signed_negative_le
    type: s8le
  - id: signed_positive_be
    type: s8be
  - id: signed_positive_le
    type: s8le
  - id: unsigned_be
    type: u8be
  - id: unsigned_le
    type: u8le
instances:
  overflow_signed_negative_be:
    pos: 48
    type: s8be
  overflow_signed_negative_le:
    pos: 56
    type: s8le
  overflow_signed_positive_be:
    pos: 64
    type: s8be
  overflow_signed_positive_le:
    pos: 72
    type: s8le
  overflow_unsigned_be:
    pos: 80
    type: u8be
  overflow_unsigned_le:
    pos: 88
    type: u8le

While writing this test I noticed that signed_negative_be and signed_negative_le were wrong. See
js_overflow.bin.zip for the binary file.

Also I misnamed my branch, it is the low bytes that overflow. The high bytes can't ever overflow because the sign bit is always set. I'll fix that and then write up a better explanation of the problem.

[cherue]

Let's look at Number.MIN_SAFE_INTEGER or -9007199254740991 or 0x ff e0 00 00 00 00 00 01 (big-endian) to demonstrate the problem.

First, the s8 is read as two u4s:

var high = this.readU4be(); // 0x ff e0 00 00
var low  = this.readU4be(); // 0x 00 00 00 01

Then high and low are XORed with 0x ff ff ff ff:

high = high ^ 0xffffffff; // 0x 00 1f ff ff
low  = low  ^ 0xffffffff; // 0x ff ff ff fe

And finally they are combined to create the s8:

return -(0x100000000 * high + low) - 1; 

The expected result is -9007199254740991 but currently the result is -9007194959773695 (WebIDE has an outdated runtime so it's still treated as a positive number).

This happens because the result of XOR is defined to be a signed 32-bit integer. Which means the final step, which should be:

return -(0x100000000 * 2097151 + 4294967294) - 1;
// -9007199254740991

currently is:

return -(0x100000000 * 2097151 + -2) - 1;
// -9007194959773695

[cherue]

@GreyCat do you want a test for this first?

How about an integer_random test? It would be a binary file filled with random data and then instances with all integer types and pos: 0 and repeat: eos. That should catch most edge cases. The KST and the tests would be huge though.

[generalmimon]

Always use strict equality operator === (or !== negated) in JavaScript unless you have a very good reason to use loose equality op with type juggling (==/!=).

Suggested change

	if ((high & 0x80000000) != 0) {
	if ((high & 0x80000000) !== 0) {

The loose equality op == knows several ways how to surprise you: at random "" == false, "" == 0, [] == false (whereas ![] === false actually) , [] == '', [] == 0, [0] == false, [1] == true, [null, null] == "," etc. Thus it's rarely a good idea to use the loose equality.

It doesn't matter whether you think you know the type of the operands, because it's JavaScript. Unless you are doing typeof X === ... everywhere, you don't know the types.

[generalmimon]

Please don't use exponentiation operator in JavaScript, because this would noticeably impair the KS runtime compatibility with JavaScript environments (compare compatibility table on MDN of exponentiation ** and Uint8Array, which is probably the latest JS feature the runtime uses).

	Chrome	Edge	Firefox	Internet Explorer	Opera	Safari	Android webview	Chrome for Android	Firefox for Android	Opera for Android	Safari on iOS	Samsung Internet	Node.js
Uint8Array	7	12	4	10	11.6	5.1	4	18	4	12	4.2	1.0	0.10
Exponentiation (**)	52	14	52	No	39	10.1	51	52	52	41	10.3	6.0	7.0.0

Moreover, environments that don't know about exponential operator ** treat its usage anywhere in the code as a syntax error, regardless of whether the code using it would run or not. So it's impossible to polyfill it in such environments, because it's a syntax thing. This is a significant difference from Uint8Array, which can be polyfilled.

I believe that in theory you can bring the current KS runtime up and running even in ancient browsers like IE 5 🙂, if you link a few polyfills.

I suggest a cleaner (and probably also faster) way how to convert the 32-bit signed integer to an unsigned one - using the unsigned right shift operator. From MDN:

Unlike the other bitwise operators, zero-fill right shift returns an unsigned 32-bit integer.

So it's enough to do >>> 0, which is guaranteed to yield a 32-bit unsigned integer. You don't even have to check if the original number is positive or negative.

Suggested change

	low = low < 0 ? 2**32 + low : low;
	low >>>= 0; // convert to unsigned 32-bit integer