Open cherue opened 4 years ago
@cherue, can you clarify which test shows the problem in JS right now?
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.
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 u4
s:
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
@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.
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
u4
s to ones8
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.