fix undefined behavior: clamp shift in B44 Decompress

[peterhillman]

Fix undefined behavior with overflow as described in #821
This fix at least suppresses the error in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=24787

A better fix may be possible, and it would be good to confirm the overflow only arises with specially crafted input files, and can never occur with images written via the OpenEXR library.

Signed-off-by: Peter Hillman peterh@wetafx.co.nz

[cary-ilm]

The TODO comment is confusing, what do you mean by 'unverified'?

[cary-ilm]

Why 26, since the type is unsigned short? Why not clamp to 10 instead, since any more would overflow the 16 bits and produce the same 0 results?

[peterhillman]

I suppose that's why I felt the need to say the fix was 'unverified' as wasn't fully sure.

IlmImfTest produces shift values of 11, so 10 is definitely not enough. It may actually need to be 31 to be sure this change won't change the decoding of broken images. shift is used to compute bias, as well as in the following lines.

My understanding is this:

All the << operators cause casts to 32 bit signed integers for those lines. That means all those lines involve 32 bit computation, which is then truncated to 16 bits. I think the implicit cast to signed arithmetic may have been unintentional.

It appears that what happens when a<<b overflows is undefined if a is signed, but defined if a is unsigned: the top bits are lost. However if b is equal to or greater than type's bit size (e.g. larger than 31 if a is 32 bit int) then the behavior is always undefined. Perhaps some 32-bit processors only take the bottom 5 bits of b, so a<<33 gives the same result as a<<1

A more conservative approach would be to clamp shift to 31, and switch all the computation to unsigned operations. This would prevent any undefined behavior. Overflows would still happen but now have well defined behavior.

[cary-ilm]

LGTM