On Aug 14 2025, at 9:46 am, David Rowley <dgrowleyml@gmail.com> wrote:
> On Fri, 15 Aug 2025 at 01:21, Greg Burd <greg@burd.me> wrote:
>> I've been working on Bitmapset and while creating a test suite for it I
>> found that there is a missing bounds check in bms_prev_member(). The
>> function takes the prevbit argument and converts it to an index into the
>> words array using WORDNUM() without checking to ensure that prevbit is
>> within the bounds of the possible values (e.g. nwords *
>> BITS_PER_BITMAPWORD) in the set. This means that $subject resulting in
>> a confusing return value when the expected value should be the highest
>> bit set.
>
> There's a comment saying:
>
> * "prevbit" must NOT be more than one above the highest possible bit
> that can
> * be set at the Bitmapset at its current size.
>
> So looks like it's the fault of the calling code and not an issue with
> bms_prev_member().
>
> David
Hi David, thanks for your feedback. You're right, there is that
statement in the comment.
There are three paths that I can see:
1) forget about it and let the comment do the work, caller beware
2) add an assert that ensures we catch errant callers and leave the
comment as is
3) add the test and adjust the value of prevbit to be in bounds and
remove the comment
I'm not in favor of (1) because I think adding either (2) or (3)
prevent, or help identify, unintentional bugs. I guess I'm a bit
overly cautious when I find code that can read outside the bounds of its
intended memory returning seemingly valid results or in other cases
potentially reading invalid memory addresses and raise a signal. That
just feels wrong.
My vote now is (2) in the attached patch with a bit fix also in the comment.
my $0.02. :)
-greg
