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

Greg Burd <greg@burd.me>

From: Greg Burd <greg@burd.me>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: David Rowley <dgrowleyml@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2025-08-14T15:37:28Z
Lists: pgsql-hackers

Attachments

On Aug 14 2025, at 11:14 am, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> David Rowley <dgrowleyml@gmail.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.
> 
> 			regards, tom lane


Thanks Tom, David,

Seems I also forgot about the case where the Bitmapset passed is NULL. 
The new assert needs to handle that as well.

-greg

Commits

  1. Add Asserts to validate prevbit values in bms_prev_member