MIME-Version: 1.0
References: 
 <CAApHDvq0T=iJ0Sf5TNE9yyWwfOeVjmrBt0wSywDnGD9Y4YJQBA@mail.gmail.com>
 <0BF8BC4F-ED7D-445F-8BB3-2336F1E17CB4@gmail.com>
In-Reply-To: <0BF8BC4F-ED7D-445F-8BB3-2336F1E17CB4@gmail.com>
From: jie wang <jugierwang@gmail.com>
Date: Thu, 2 Apr 2026 15:54:18 +0800
Message-ID: 
 <CAJnZyeBNEVqmX2YByQt-zpo9UiOm6=jE9KdZSfNeHtOeQ1mSWw@mail.gmail.com>
Subject: Re: Small and unlikely overflow hazard in bms_next_member()
To: Chao Li <li.evan.chao@gmail.com>
Cc: David Rowley <dgrowleyml@gmail.com>,
	PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="000000000000cea9e3064e7582bc"
Archived-At: 
 <https://www.postgresql.org/message-id/CAJnZyeBNEVqmX2YByQt-zpo9UiOm6%3DjE9KdZSfNeHtOeQ1mSWw%40mail.gmail.com>
Precedence: bulk

--000000000000cea9e3064e7582bc
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Chao Li <li.evan.chao@gmail.com> =E4=BA=8E2026=E5=B9=B44=E6=9C=882=E6=97=A5=
=E5=91=A8=E5=9B=9B 14:39=E5=86=99=E9=81=93=EF=BC=9A

>
>
> > On Apr 2, 2026, at 12:09, David Rowley <dgrowleyml@gmail.com> wrote:
> >
> > I've been working on bms_left_shift_members() to bitshift members
> > either left or right in order to tidy up some existing code and
> > improve a future Bitmapset use case I'm currently working on.
> >
> > When testing some ERROR code I added to ensure we don't get an
> > excessively large left shift value and end up with members higher than
> > INT32_MAX, I discovered that bms_next_member() can't handle that
> > value, as "prevbit++" will wrap to INT32_MIN and then we'll try to
> > access a negative array index, i.e. seg fault.
> >
> > I appreciate that such a large member is quite unlikely, but if this
> > isn't fixed then I need to code my error checking code to disallow
> > members >=3D INT32_MAX rather than > INT32_MAX. I did have a comment
> > explaining why I was doing that, but fixing the bug saves the weird
> > special case and the comment.
> >
> > Patched attached. I was thinking it might not be worthy of
> > backpatching, but I'll entertain alternative views on that.
> >
> > David
> > <bms_next_member_fix.patch>
>
> Both the return value of bms_next_member() and the parameter =E2=80=9Cpre=
vbit" are
> defined as int, which seems to imply that a bitmap can hold at most
> INT32_MAX elements. So I wonder if a cleaner fix would be to just add a
> range guard, like this:
>
> ```
> diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.=
c
> index 786f343b3c9..7f79f81f278 100644
> --- a/src/backend/nodes/bitmapset.c
> +++ b/src/backend/nodes/bitmapset.c
> @@ -1294,7 +1294,7 @@ bms_next_member(const Bitmapset *a, int prevbit)
>
>         Assert(bms_is_valid_set(a));
>
> -       if (a =3D=3D NULL)
> +       if (a =3D=3D NULL || prevbit =3D=3D INT32_MAX)
>                 return -2;
>         nwords =3D a->nwords;
>         prevbit++;
> ```
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>
>
>
>
Hi,

The both solutions look good to me. I am slightly keen on Chao's version
that looks simpler to me.

Thanks!
wang jie

--000000000000cea9e3064e7582bc
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote g=
mail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">Chao Li &lt;<a =
href=3D"mailto:li.evan.chao@gmail.com">li.evan.chao@gmail.com</a>&gt; =E4=
=BA=8E2026=E5=B9=B44=E6=9C=882=E6=97=A5=E5=91=A8=E5=9B=9B 14:39=E5=86=99=E9=
=81=93=EF=BC=9A<br></div><blockquote class=3D"gmail_quote" style=3D"margin:=
0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">=
<br>
<br>
&gt; On Apr 2, 2026, at 12:09, David Rowley &lt;<a href=3D"mailto:dgrowleym=
l@gmail.com" target=3D"_blank">dgrowleyml@gmail.com</a>&gt; wrote:<br>
&gt; <br>
&gt; I&#39;ve been working on bms_left_shift_members() to bitshift members<=
br>
&gt; either left or right in order to tidy up some existing code and<br>
&gt; improve a future Bitmapset use case I&#39;m currently working on.<br>
&gt; <br>
&gt; When testing some ERROR code I added to ensure we don&#39;t get an<br>
&gt; excessively large left shift value and end up with members higher than=
<br>
&gt; INT32_MAX, I discovered that bms_next_member() can&#39;t handle that<b=
r>
&gt; value, as &quot;prevbit++&quot; will wrap to INT32_MIN and then we&#39=
;ll try to<br>
&gt; access a negative array index, i.e. seg fault.<br>
&gt; <br>
&gt; I appreciate that such a large member is quite unlikely, but if this<b=
r>
&gt; isn&#39;t fixed then I need to code my error checking code to disallow=
<br>
&gt; members &gt;=3D INT32_MAX rather than &gt; INT32_MAX. I did have a com=
ment<br>
&gt; explaining why I was doing that, but fixing the bug saves the weird<br=
>
&gt; special case and the comment.<br>
&gt; <br>
&gt; Patched attached. I was thinking it might not be worthy of<br>
&gt; backpatching, but I&#39;ll entertain alternative views on that.<br>
&gt; <br>
&gt; David<br>
&gt; &lt;bms_next_member_fix.patch&gt;<br>
<br>
Both the return value of bms_next_member() and the parameter =E2=80=9Cprevb=
it&quot; are defined as int, which seems to imply that a bitmap can hold at=
 most INT32_MAX elements. So I wonder if a cleaner fix would be to just add=
 a range guard, like this:<br>
<br>
```<br>
diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c<=
br>
index 786f343b3c9..7f79f81f278 100644<br>
--- a/src/backend/nodes/bitmapset.c<br>
+++ b/src/backend/nodes/bitmapset.c<br>
@@ -1294,7 +1294,7 @@ bms_next_member(const Bitmapset *a, int prevbit)<br>
<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 Assert(bms_is_valid_set(a));<br>
<br>
-=C2=A0 =C2=A0 =C2=A0 =C2=A0if (a =3D=3D NULL)<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (a =3D=3D NULL || prevbit =3D=3D INT32_MAX)<=
br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -2;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 nwords =3D a-&gt;nwords;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 prevbit++;<br>
```<br>
<br>
Best regards,<br>
--<br>
Chao Li (Evan)<br>
HighGo Software Co., Ltd.<br>
<a href=3D"https://www.highgo.com/" rel=3D"noreferrer" target=3D"_blank">ht=
tps://www.highgo.com/</a><br>
<br>
<br>
<br>
<br>
<br>
<br></blockquote><div><br></div><div>Hi,</div><div><br></div><div>The both =
solutions look good to me. I am slightly keen on Chao&#39;s version that lo=
oks simpler to me.</div><div><br></div><div>Thanks!</div><div>wang jie</div=
></div></div>

--000000000000cea9e3064e7582bc--