On Sun, Jan 21, 2018 at 9:15 PM, Jann Horn <jannh@xxxxxxxxxx> wrote: > [whoops, resending as non-HTML mail] > > On Sun, Jan 21, 2018 at 11:40 AM, Russell King - ARM Linux > <linux@xxxxxxxxxxxxxxx> wrote: >> On Sat, Jan 20, 2018 at 01:06:09PM -0800, Dan Williams wrote: >>> +/* >>> + * If idx is negative or if idx > size then bit 63 is set in the mask, >>> + * and the value of ~(-1L) is zero. When the mask is zero, bounds check >>> + * failed, array_ptr will return NULL. >> >> The more times I see this the more times I'm unhappy with this comment: >> >> 1. does this really mean "idx > size" or "idx >= size" ? The code >> implements the latter not the former. > > Copying the code here for context: > return ~(long)(idx | (sz - 1 - idx)) >> (BITS_PER_LONG - 1); > > That part of the condition (ignoring the overflow edgecases) is > equivalent to "!(idx > sz - 1)", which is equivalent to "idx <= sz - > 1", which is (ignoring overflow edgecases) equivalent to "idx < sz". > > Handling of edgecases: > idx>=2^(BITS_PER_LONG-1) will cause a NULL return through the first > part of the condition. > Hmm... a problematic case might be "sz==0 && > idx==2^(BITS_PER_LONG-1)-1". The first part of the expression wouldn't > trigger, the second part would be > "2^(BITS_PER_LONG)-1-(2^(BITS_PER_LONG-1)-1) == > 2^(BITS_PER_LONG)-2^(BITS_PER_LONG-1) == 2^(BITS_PER_LONG-1)", which > also wouldn't trigger, I think? Er, of course 2^(BITS_PER_LONG-1) still has the high bit set. Sorry for the noise.
