Skip to content

Commit 77a2746

Browse files
fingolfinclaude
andcommitted
kernel: Avoid out-of-range shifts in CopyBits
CopyBits and its helpers shift by amounts derived from <frombit>, <tobit> and the alignment difference between the two blocks. Several of those can reach BIPEB, where `1 << BIPEB` and `x >> BIPEB` are undefined: * After a partial first destination word is filled, <frombit> can be left equal to BIPEB rather than wrapping to the next source word. MaskForCopyBits then evaluates `(UInt)1 << BIPEB`. * In the main loop <frombit> equal to 0 makes the second half of the word assembly shift by `BIPEB - 0`. * CopyInWord shifts by <shift>. The callers do keep it in range, but only indirectly: in the `tobit + BIPEB - frombit` case it takes the branch condition `frombit + tailbits > BIPEB` to show frombit > tobit. Normalise <frombit> to the start of the next word, make MaskForCopyBits total so an out-of-range bound yields an empty mask, special-case the aligned main loop, and bound the shift in CopyInWord directly. An exhaustive sweep over frombit, tobit in [0,BIPEB) and nbits in [1,5*BIPEB] hits the undefined shift in 1953 of 1310720 cases, and gives identical results before and after: in every one of those cases the mask being computed is dead, because no whole words remain to copy. So this changes no observable behaviour on current compilers. The guards also do a second job. They are what lets a static analyzer establish the bounds: without them clang's core.BitwiseShift reports "Left shift overflows the capacity of 'UInt'" for paths reaching here from blister.c and vecgf2.c. Please keep them even where a local reading suggests the branch is dead. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 989cc35 commit 77a2746

1 file changed

Lines changed: 34 additions & 7 deletions

File tree

src/bits_intern.h

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,13 @@
3838
// constructs a mask that selects bits <from> to <to> inclusive of a UInt
3939
static inline UInt MaskForCopyBits(UInt from, UInt to)
4040
{
41-
return ((to == BIPEB - 1) ? 0 : ((UInt)1 << (to + 1))) - ((UInt)1 << from);
41+
UInt high = 0;
42+
UInt low = 0;
43+
if (to < BIPEB - 1)
44+
high = (UInt)1 << (to + 1);
45+
if (from < BIPEB)
46+
low = (UInt)1 << from;
47+
return high - low;
4248
}
4349

4450
/* copies a block of bits from the UInt <from> to the one pointed at
@@ -52,10 +58,18 @@ CopyInWord(UInt * to, UInt startbit, UInt endbit, UInt from, Int shift)
5258
{
5359
UInt m = MaskForCopyBits(startbit + shift, endbit + shift);
5460
*to &= ~m;
55-
if (shift >= 0)
56-
*to |= ((from << shift) & m);
57-
else
58-
*to |= ((from >> -shift) & m);
61+
if (shift >= 0) {
62+
UInt lshift = (UInt)shift;
63+
if (lshift >= BIPEB)
64+
return;
65+
*to |= ((from << lshift) & m);
66+
}
67+
else {
68+
UInt rshift = (UInt)(-shift);
69+
if (rshift >= BIPEB)
70+
return;
71+
*to |= ((from >> rshift) & m);
72+
}
5973
}
6074

6175

@@ -72,6 +86,8 @@ static ALWAYS_INLINE void CopyBits(const UInt * fromblock,
7286
return;
7387
GAP_ASSERT(frombit < BIPEB);
7488
GAP_ASSERT(tobit < BIPEB);
89+
if (frombit >= BIPEB || tobit >= BIPEB)
90+
return;
7591
/* If the alignment of the two data blocks matches, things are relatively
7692
* easy
7793
*/
@@ -126,13 +142,24 @@ static ALWAYS_INLINE void CopyBits(const UInt * fromblock,
126142
toblock++;
127143
nbits -= tailbits;
128144
tobit = 0;
145+
if (frombit == BIPEB) {
146+
frombit = 0;
147+
fromblock++;
148+
}
129149
}
130150

131151
// Main loop for long copies fills whole blocks of destination
152+
if (frombit >= BIPEB)
153+
return;
132154
UInt m1 = MaskForCopyBits(frombit, BIPEB - 1);
133155
while (nbits >= BIPEB) {
134-
x = (*fromblock++ & m1) >> frombit;
135-
x |= (*fromblock & ~m1) << (BIPEB - frombit);
156+
if (frombit == 0) {
157+
x = *fromblock++;
158+
}
159+
else {
160+
x = (*fromblock++ & m1) >> frombit;
161+
x |= (*fromblock & ~m1) << (BIPEB - frombit);
162+
}
136163
*toblock++ = x;
137164
nbits -= BIPEB;
138165
}

0 commit comments

Comments
 (0)