adler32 vs. crc32 inconsistency

[sjaeckel]

in crc32_test we have:

{
   const void* in = "libtomcrypt";
   const unsigned char crc32[] = { 0xef, 0x76, 0x73, 0xb3 };
   ....
}

However the correct CRC32 from "libtomcrypt" string is IMHO b37376ef not ef7673b3.

By which I mean the actual CRC32 implementation seems to be somehow incorrect not crc32_test.

[rofl0r]

b37376ef vs ef7673b3 is endian swapped

[karel-m]

@sjaeckel what do you think about this?

In my perl bindings I have implemented a fix like this: DCIT/perl-CryptX@b8bf151

I can prepare the same fix as a pull request if you agree.

[sjaeckel]

... unsigned char crc32[] = { 0xef, 0x76, 0x73, 0xb3 }; is the same as uint32_t crc32 = 0xb37376ef; on a little endian CPU (memory-wise).

c.f. #109 and I'm pretty certain that the original implementation is correct.

[karel-m]

If the implementation is correct than the interface must be wrong:

void crc32_finish(crc32_state *ctx, void *hash, unsigned long size)

I believed (but now I am in doubts) that void *hash is meant to be a pointer to unsigned char[4] (see crc32_test and unsigned char out[4]).

I do not see any reason for using void * instead I prefer explicitly declare it as unlong32 *hash or unsigned char *hash.

Other thing is that the test case (crc32_test) should be a proof-of-concept correct implementation (as it was a place where people will look to see how to use/call it).

[rofl0r]

there's http://reveng.sourceforge.net/crc-catalogue/17plus.htm#crc.cat.crc-32 which lists check=0xcbf43926 as the checksum for the test string 123456789. i suppose that should match. more info here https://www.lammertbies.nl/comm/info/crc-calculation.html
i personally would assume a crc32 function giving me an uint32_t, not an array of 4bytes, however that's not what i see for example here: https://github.com/rofl0r/libulz/blob/master/src/crc32/crc32c.c#L143 . here the output seems to be arranged to produce an endian-independent value, however that is a CRC32-C impl (castagnoli variant).

[karel-m]

Other thing is consistency between interfaces for crc32 and adler32

Both have pretty similar:

void adler32_finish(adler32_state *ctx, void *hash, unsigned long size)
void crc32_finish(crc32_state *ctx, void *hash, unsigned long size)

but the interpretation of hash output is different

//crc32_test - expected HEX value "B37376EF"
const unsigned char crc32[] = { 0xef, 0x76, 0x73, 0xb3 };

//adler32_test - expected HEX value "1BE804BA"
const unsigned char adler32[] = { 0x1b, 0xe8, 0x04, 0xba };

for test vectors try:

• http://www.miniwebtool.com/crc32-checksum-calculator/
• http://www.miniwebtool.com/adler32-checksum-calculator/

[sjaeckel]

but the interpretation of hash output is different...

oh yeah, that's pretty bad...

nonetheless I think we should keep the crc implementation and adjust the adler implementation, okay?

[karel-m]

I am open to both ways (changing crc32 or changing adler 32). First of all we should define what the output is (ulong32, 4 unsigned chars in LE order, 4 unsigned chars in BE order etc.) and change "void *" accordingly. BTW what is the crc32/adler32 interface in other libraries? My patch was based on byte order of commonly used hexadecimal representation.

[karel-m]

I will try to reword the problem which should be perhaps named adler32 vs. crc32 inconsistency:

1/ the API

We have:

void adler32_finish(adler32_state *ctx, void *hash, unsigned long size)
void crc32_finish(crc32_state *ctx, void *hash, unsigned long size)

which is not what one can see on other places in libtomcrypt. My proposal:

void adler32_finish(adler32_state *ctx, unsigned char *out, unsigned long *outlen)
void crc32_finish(crc32_state *ctx, unsigned char *out, unsigned long *outlen)

2/ the byte order

Both functions adler32_finish and crc32_finish return 4 bytes (the implementation guarantees the same result on all platforms independant on endianness), however:

• adler32_finish returns 4 bytes in big endian order AKA network byte order
• crc32_finish returns 4 bytes in little endian order

My proposal: change crc32_finish to return network byte order

[sjaeckel]

1/ the API

These API's didn't fit anywhere, that's why I broke with the old concepts. (Yes I know, I could've integrated them in the existing "hashing framework"... I can't exactly remember why I didn't do that, but I somehow still don't like the idea to have them in there... why didn't I integrate them there again...?)

TBH I don't care if we write the written length back, but we definitely have to stop with that unsigned char* craziness for opaque data at one point. Therefor 👎 for unsigned char* out

...and 👍 for a PR "change all in- and output data pointers to void*" (just kiddin'!)

I also thought about something like the following, but apparently that's not an option if you think about integrating other checksum/nc-hashing algorithms with bigger output.

ulong32 X_finish(X_state *ctx);

2/ the byte order

👍

[karel-m]

Ad 2/
For "libtomcrypt" the correct values are IMO:

• crc32: B37376EF
• adler32: 1BE804BA

So in our test suit we need:

• const unsigned char crc32[] = { 0xb3, 0x73, 0x76, 0xef };
• const unsigned char adler32[] = { 0x1b, 0xe8, 0x04, 0xba };

Which can be achieved by b3109cb (== merging fix/122)

Ad 1/
let's keep the current API

[coveralls]

Coverage remained the same at 87.319% when pulling b3109cb on fix/122 into 456908e on develop.

[sjaeckel]

fine to rebase&merge