| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | David Rowley <dgrowleyml(at)gmail(dot)com> |
| Cc: | PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Small and unlikely overflow hazard in bms_next_member() |
| Date: | 2026-04-02 06:38:46 |
| Message-ID: | 0BF8BC4F-ED7D-445F-8BB3-2336F1E17CB4@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Apr 2, 2026, at 12:09, David Rowley <dgrowleyml(at)gmail(dot)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 >= 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 “prevbit" 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 == NULL)
+ if (a == NULL || prevbit == INT32_MAX)
return -2;
nwords = a->nwords;
prevbit++;
```
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Hayato Kuroda (Fujitsu) | 2026-04-02 06:57:10 | RE: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE |
| Previous Message | Amit Kapila | 2026-04-02 06:34:18 | Re: Use SIGTERM instead of SIGUSR1 for slotsync worker to exit during promotion? |