kaitai-io / kaitai_struct_javascript_runtime

Kaitai Struct: runtime for JavaScript
Apache License 2.0
37 stars 22 forks source link

bitwise integer for 32bits returns signed value as default instead of unsigned value #22

Closed gauravlanjekar closed 2 years ago

gauravlanjekar commented 2 years ago

Hello, I recently discovered that the bitwise integers behave differently for 32 bits Following is my ksy

meta:
  id: testme
  endian: be

seq:
  - id: something
    type: something
types:
  something:
    seq:
      - id: val
        type: b32

if I pass in c0 00 00 02 it evaluates to -1073741822 rather than the unsigned value 3221225474

b16 works as expected. passing in c0 00 evaluates to 49152

generalmimon commented 2 years ago

Good catch! Looks like I screwed this up in d9bc0b9, unfortunately, because I changed this:

  mask <<= shiftBits;
  // derive reading result
  var res = (this.bits & mask) >>> shiftBits;

(where res is guaranteed to be unsigned, because it's processed by the >>> operator that returns unsigned 32-bit integers from 0 to 0xFFFF_FFFF = 4_294_967_295)

...to this:

  var res = (this.bits >>> shiftBits) & mask;

(the last bitwise operator is & , which always returns signed 32-bit integers from -0x8000_0000 = -2_147_483_648 to 0x7FFF_FFFF = 2_147_483_647)

I thought it's an equivalent change, but now I see it isn't.


We totally need a test for this in the KST suite.

I feel like it'd be worth it to write a fuzzer for these bit integer types with different bit layouts (e.g. b1 + b31, b16 + b16, etc.) and evaluate on random data or some special candidates suspected of breaking something, e.g. ff ff ff ff, 7f ff ff ff, 80 00 00 00 etc. in all target languages if the reading methods work in all cases. I'm almost certain that we'll learn something about bitwise operators/integer types in some target language that we didn't know :-)

One would say that the methods readBitsInt{Be,Le}() are only a few lines of code long, so what could possibly go wrong there, right? Well, almost everything - for example, subtracting 1 from the number (1 << 63) is a problem in PHP (https://github.com/kaitai-io/kaitai_struct_php_runtime/commit/c0cc3e2af72ee4495741b23329457be6e153aa4b), because (1 << 63) is the lowest possible integer value in PHP (PHP_INT_MIN, int(-9223372036854775808)) and trying to go below it will cause the number to implicitly convert to float and lose precision (i.e. int(-9223372036854775808) - 1 === float(-9.2233720368548E+18)). When the target languages have such quirks, it's hard to predict what problems might occur - so I think some "brute-force" testing method would make sense here.

gauravlanjekar commented 2 years ago

Ahh.. Right. ! .. So I guess I will need to downgrade for the moment until this is sorted. Do you recommend a version ?

mfalkvidd commented 2 years ago

@gauravlanjekar the latest commit before generalmimon's chage is https://github.com/kaitai-io/kaitai_struct_javascript_runtime/tree/058a3ce31fa8c3b08edd79c975b068280ca457cf