CRC sometimes returns negative value #21

New Issue

2022-08-04T05:33:12Z

Crinax commented

2022-08-04 05:33:12 +00:00

(Migrated from github.com)

So, I know that this problem has already been raised more than once.
Let's give an example, take a png file in which the checksum is calculated using exactly the same algorithm. There is data:

[0x49, 0x48, 0x44, 0x52, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, 0x00, 0x20, 0x08, 0x02, 0x00, 0x00, 0x00]

In the file, this data has the following CRC: FC18EDA3

The same algorithm returns a negative value: -3E7125D

If you look at other implementations of this algorithm, you will notice that they all return an unsigned value. Even in Python, this problem has already been fixed:

from zlib import crc32

crc32(b"SheetJS") # 2647669026

For example, unsigned numbers are used here so that the algorithm works correctly:
http://stigge.org/martin/pub/SAR-PR-2006-05.pdf

Maybe it's worth using >>> 0 to work correctly?
Or make a separate function for unsigned calculation🤔

So, I know that this problem has already been raised more than once. Let's give an example, take a png file in which the checksum is calculated using exactly the same algorithm. There is data: ```js [0x49, 0x48, 0x44, 0x52, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, 0x00, 0x20, 0x08, 0x02, 0x00, 0x00, 0x00] ``` In the file, this data has the following CRC: `FC18EDA3` The same algorithm returns a negative value: `-3E7125D` If you look at other implementations of this algorithm, you will notice that they all return an unsigned value. Even in Python, this problem has already been fixed: ```python from zlib import crc32 crc32(b"SheetJS") # 2647669026 ``` For example, unsigned numbers are used here so that the algorithm works correctly: http://stigge.org/martin/pub/SAR-PR-2006-05.pdf Maybe it's worth using `>>> 0` to work correctly? Or make a separate function for unsigned calculation🤔

SheetJSDev commented

2022-08-04 05:57:32 +00:00

(Migrated from github.com)

First, the README states that the result is a signed 32 bit integer.

The main reason for sticking with signed 32-bit integers is performance.

V8 (Node/Chrome JS engine) can optimize for "Smi" (small integers). On a 64-bit platform, Smi is signed 32-bit integer. V8 deoptimizes when vacillating between 32-bit signed ints and integers not in the range -231 .. 231 - 1 . This was discussed a bit in https://github.com/SheetJS/js-crc32/issues/4#issuecomment-170050796

The >>> 0 part is easy enough not to merit a separate function.

It probably makes sense to add a note in the README about how to coerce to 32-bit signed integer (x | 0) and how to coerce to 32-bit unsigned integer (x >>> 0), and we'd accept a PR for that.

First, the README states that the result is a signed 32 bit integer. The main reason for sticking with signed 32-bit integers is performance. V8 (Node/Chrome JS engine) can optimize for "Smi" (small integers). On a 64-bit platform, Smi is signed 32-bit integer. V8 deoptimizes when vacillating between 32-bit signed ints and integers not in the range -2**31 .. 2**31 - 1 . This was discussed a bit in https://github.com/SheetJS/js-crc32/issues/4#issuecomment-170050796 The `>>> 0` part is easy enough not to merit a separate function. It probably makes sense to add a note in the README about how to coerce to 32-bit signed integer (`x | 0`) and how to coerce to 32-bit unsigned integer (`x >>> 0`), and we'd accept a PR for that.

SheetJSDev commented

2022-08-13 04:31:50 +00:00

(Migrated from github.com)

GitHub markdown unfortunately lacks admonitions, but the sentence "The return value is a signed 32-bit integer!" has been bolded and https://github.com/SheetJS/js-crc32#signed-integers includes some conversion notes. Please let us know how we can further improve the docs!

Sign in to join this conversation.