Re: [PATCH] bms_prev_member() can read beyond the end of the array of allocated words

On Fri, 15 Aug 2025 at 03:14, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> > It is valid to pass prevbit as a->nwords * BITS_PER_BITMAPWORD as the
> > code does "prevbit--;". Maybe it would be less confusing if it were
> > written as:
> > * "prevbit" must be less than or equal to "a->nwords * BITS_PER_BITMAPWORD".
> > The Assert should be using <= rather than <.
>
> Actually, I don't agree with that. It's true that it wouldn't fail,
> but a caller doing that is exhibiting undue intimacy with the innards
> of Bitmapsets. The expected usage is that the argument is initially
> -1 and after that the result of the previous call (which'll
> necessarily be less than a->nwords * BITS_PER_BITMAPWORD). We don't
> have any state with which we can verify the chain of calls, but it
> seems totally reasonable to me to disallow an outside caller
> providing an argument >= a->nwords * BITS_PER_BITMAPWORD.

I can't get on board with this way of thinking. I'm happy enough that
we add an Assert, but if we're going to do that then the Assert should
be coded in a way that's aligned to the actual restriction that we're
trying to protect against. I've heard people arguing before that
Asserts can act as documentation of what are valid values for a given
function parameter. If that's true, then we should code the Assert so
it checks for *valid* values, not 1 less than a valid value. IMO,
doing it any other way is likely to cause confusion in people who want
to use the function, or bug reports that we're off by 1 in the Assert.

David