CommonBits::zeroLowerBits undefined behavior

[goatbar]

It is possible to get nBits to be a shift larger than 1 << 63

I don't have a cleaned up test case for this issue. I will try to add one to this issue soon.

This is probably not the right fix...

/*static public*/
int64
CommonBits::zeroLowerBits(int64 bits, int nBits)
{
        if (nBits >= sizeof(nBits) * 8) return 0;
        int64 invMask = (1<< nBits)-1;
        int64 mask = ~ invMask;
        int64 zeroed = bits & mask;
        return zeroed;
}

Found with autofuzz.

[Paul Ramsey <pramsey@…>]

In 6c5b169/git:

Handle nbits > 37
References #869

[Paul Ramsey <pramsey@…>]

In 2c9bfe8/git:

Handle nbits > 63
References #869

[Paul Ramsey <pramsey@…>]

In b8a51cb/git:

Handle nbits > 63
3.6 branch
Closes #869

[rouault]

Actually, the fix is incorrect for several reasons:

• (1<< nBits)-1 has only int operands, so will be evaluated on 32 bit only. Consequently nBits >= 32 will result in incorrect behaviour, unless you cast the first 1 to int64 : ((int64)1 << nBits) - 1
• but, if let to 32 bits ( (1<< nBits)-1 ), then nBits == 31 is also an invalid input (-fsanitize=undefined warns about "runtime error: left shift of 1 by 31 places cannot be represented in type 'int'") even if "sane" compilers / most CPUs will finally do the right thing. The right fix in that case to avoid undefined/implementation defined behaviour is to use a unsigned 1, so (1U << nBits) - 1
• for the same reason, is the mask is intended to be a 64 bit one, so ((int64)1 << nBits) - 1), then nBits == 63 also triggers undefined behaviour. And thus the right fix would be ((unsigned int64)1 << nBits) - 1

[Paul Ramsey <pramsey@…>]

In 311b64a/git:

Improve the bit-shifting logic with good types and so on
Even Roualt, Kurt Schwehr
References #869

[Paul Ramsey <pramsey@…>]

In 3e8be5b/git:

Improve the bit-shifting logic with good types and so on
Even Roualt, Kurt Schwehr
References #869

[Paul Ramsey <pramsey@…>]

In 9362bf2/git:

Improve the bit-shifting logic with good types and so on
Even Roualt, Kurt Schwehr
References #869

[nila]

@pramsey I noticed the initial commits didn't fall out well on some builds.

I could not reproduce the failure, but perhaps using std::uint64_t would make any difference.

[goatbar]

Thanks!