Skip to content

Fix overflow when reading negative s8 #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix overflow when reading negative s8 #16

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
edc3dad
Fix overflow when reading negative s8
cherue Mar 15, 2020
7c0de72
Remove overflow check for high bytes in s8
cherue Mar 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions KaitaiStream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,15 +214,9 @@ KaitaiStream.prototype.readS4be = function(e) {
*/
KaitaiStream.prototype.readS8be = function(e) {
this.ensureBytesLeft(8);
var v1 = this.readU4be();
var v2 = this.readU4be();

if ((v1 & 0x80000000) != 0) {
// negative number
return -(0x100000000 * (v1 ^ 0xffffffff) + (v2 ^ 0xffffffff)) - 1;
} else {
return 0x100000000 * v1 + v2;
}
var high = this.readU4be();
var low = this.readU4be();
return KaitaiStream.twoU4sToS8(high, low);
};

// ........................................................................
Expand Down Expand Up @@ -260,15 +254,9 @@ KaitaiStream.prototype.readS4le = function(e) {
*/
KaitaiStream.prototype.readS8le = function(e) {
this.ensureBytesLeft(8);
var v1 = this.readU4le();
var v2 = this.readU4le();

if ((v2 & 0x80000000) != 0) {
// negative number
return -(0x100000000 * (v2 ^ 0xffffffff) + (v1 ^ 0xffffffff)) - 1;
} else {
return 0x100000000 * v2 + v1;
}
var low = this.readU4le();
var high = this.readU4le();
return KaitaiStream.twoU4sToS8(high, low);
};

// ------------------------------------------------------------------------
Expand Down Expand Up @@ -802,6 +790,18 @@ KaitaiStream.createStringFromArray = function(array) {
return chunks.join("");
};

KaitaiStream.twoU4sToS8 = function(high, low) {
if ((high & 0x80000000) != 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// negative number
high = high ^ 0xffffffff;
low = low ^ 0xffffffff;
low = low < 0 ? 2**32 + low : low;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

return KaitaiStream;

}));