Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
On Mon, Jan 15, 2018 at 9:37 PM Niels Möller nisse@lysator.liu.se wrote:
unsigned overflow = b2 & 0x8000000000000000;b1 <<= 1;b2 <<= 1;if (overflow)b1 |= 0x01;I'd write the shift as
b1 = (b1 << 1) | (b2 >> 63); b2 <<= 1;
I have bad experience with shifts over 31. I could switch it to that, but my understanding is that they don't universally work especially in 32-bit systems offerring 64-bit quantities. Given that this code is being used unconditionally and is not performance critical, I'd stay with the safe version, unless you have strong opinion about it.
What problems have you seen? I've not heard of any C compilers which have uint64_t but doesn't support shift properly (and this is the simplest case, with unsigned values and constant shift count). The idiom I proposed is used all over the place in gmp, but there I guess we don't use 64-bit types on 32-bit platforms.
On the other hand, I think
unsigned overflow = b2 & 0x8000000000000000;
is incorrect if int is 32 bit, won't the result be always zero?
In your updated patch you write
+ if (overflow) + b1 |= 0x01; + + if (in->b[0] & 0x80) + b2 ^= 0x87;
It would be nice to at least handle the twp top bits in the same way. If you're worried 64-bit shifts are not supported by 32-bit compilers, maybe replace "overflow" by "in->b[8] & 0x80", for consistency ?
And it would be good with proper test coverage for all four values of the two bits in questions.
They are updated to: if (length == 16) { encrypt(key, 16, out, ctx->Y.b); } else { encrypt(key, 16, tmp, ctx->Y.b); memcpy(out, tmp, length); }
The difference is that tmp is used to store the output of encrypt() in the second clause.
Sorry for the confusion. I read "length" as the length of the final partial block, but here it's the desired digest size, and the memcpy is needed for truncation.
Regards, /Niels