Обсуждение: Post-release followup: pg_add_size_overflow()

Hi all,

The fix for CVE-2025-12818 introduced a few identical copies of size_t
addition, and now that we've released, I'd like to pull those back
into shape.

0001 replaces the bespoke code with a new size_t implementation of the
operators in common/int.h. 0002 additionally makes use of these in
shmem.c, because I couldn't think of a good reason not to.

Couple things to note:

1) The backend's add_size(), which I patterned the CVE fix on
originally, checks if the result is less than either operand. The
common/int.h implementations check only the *first* operand, which
also looks correct to me -- if (result < a), it must also be true that
(result < b), because otherwise (result - b) is nonnegative and we
couldn't have overflowed the addition in the first place. But my brain
is a little fried from looking at these problems, and I could use a +1
from someone with fresh eyes.

2) I have not implemented pg_neg_size_overflow(), because to me it
seems likely to be permanently dead code, and it would require
additional reasoning about the portability of SSIZE_MAX.
(pg_sub_size_overflow(), by contrast, is easy to do and feels like it
might be useful to someone eventually.)

I don't currently plan to backport this, because I don't think the
delta is likely to cause anyone additional pain in the future, but let
me know if you disagree.

Thanks!
--Jacob

Hi, Jacob,

I just reviewed the patch. Overall looks solid to me. Just a small comments:

> On Nov 19, 2025, at 04:09, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
>
> Hi all,
>
> The fix for CVE-2025-12818 introduced a few identical copies of size_t
> addition, and now that we've released, I'd like to pull those back
> into shape.
>
> 0001 replaces the bespoke code with a new size_t implementation of the
> operators in common/int.h. 0002 additionally makes use of these in
> shmem.c, because I couldn't think of a good reason not to.
>
> Couple things to note:
>
> 1) The backend's add_size(), which I patterned the CVE fix on
> originally, checks if the result is less than either operand. The
> common/int.h implementations check only the *first* operand, which
> also looks correct to me -- if (result < a), it must also be true that
> (result < b), because otherwise (result - b) is nonnegative and we
> couldn't have overflowed the addition in the first place. But my brain
> is a little fried from looking at these problems, and I could use a +1
> from someone with fresh eyes.
>
> 2) I have not implemented pg_neg_size_overflow(), because to me it
> seems likely to be permanently dead code, and it would require
> additional reasoning about the portability of SSIZE_MAX.
> (pg_sub_size_overflow(), by contrast, is easy to do and feels like it
> might be useful to someone eventually.)
>
> I don't currently plan to backport this, because I don't think the
> delta is likely to cause anyone additional pain in the future, but let
> me know if you disagree.
>
> Thanks!
> --Jacob
> <0001-Add-pg_add_size_overflow-and-friends.patch><0002-postgres-Use-pg_-add-mul-_size_overflow.patch>

1 - 0001
```
+/*
+ * pg_neg_size_overflow is currently omitted, to avoid having to reason about
+ * the portability of SSIZE_MIN/_MAX before a use case exists.
+ */
+#if 0
+static inline bool
+pg_neg_size_overflow(size_t a, ssize_t *result)
+{
+    ...
+}
+#endif
```

Putting “…” inside a function body looks quite uncommon. I searched over the source tree and couldn't find other
occurrence.As the comment has explained why omitting pg_neg_size_overflow, maybe just remove the entry #if 0 block, or
justleave an empty function body. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Tue, Nov 18, 2025 at 12:09:17PM -0800, Jacob Champion wrote:
> The fix for CVE-2025-12818 introduced a few identical copies of size_t
> addition, and now that we've released, I'd like to pull those back
> into shape.

Yes, I've noticed these TODOs in the final patch.  Thanks for the
follow-up cleanup.

> 0001 replaces the bespoke code with a new size_t implementation of the
> operators in common/int.h. 0002 additionally makes use of these in
> shmem.c, because I couldn't think of a good reason not to.

Sounds good to me.

> Couple things to note:
>
> 1) The backend's add_size(), which I patterned the CVE fix on
> originally, checks if the result is less than either operand. The
> common/int.h implementations check only the *first* operand, which
> also looks correct to me -- if (result < a), it must also be true that
> (result < b), because otherwise (result - b) is nonnegative and we
> couldn't have overflowed the addition in the first place. But my brain
> is a little fried from looking at these problems, and I could use a +1
> from someone with fresh eyes.

This is following the same pattern as what has been introduced in
7dedfd22b798 for the other unsigned types in int.h.  Anyway, looking
at that separately, the logic of 0001 seems right here.

> 2) I have not implemented pg_neg_size_overflow(), because to me it
> seems likely to be permanently dead code, and it would require
> additional reasoning about the portability of SSIZE_MAX.
> (pg_sub_size_overflow(), by contrast, is easy to do and feels like it
> might be useful to someone eventually.)

Documenting the portability issue is important, indeed.  I'd suggest
to not use a ifdef 0, though, which may be confusing on grep if one
does not look at the surrounding lines.  Leaving that in the shape of
a comment would be hard to miss.  Anyway, are you worrying about
SIZE_MAX matching with something different than the compile-time value
at runtime?  If we don't have use for a neg subroutine yet, leaving
that out of the picture for now is fine here.

> I don't currently plan to backport this, because I don't think the
> delta is likely to cause anyone additional pain in the future, but let
> me know if you disagree.

Keeping this cleanup on HEAD sounds fine to me.
--
Michael

On Tue, Nov 18, 2025 at 5:17 PM Chao Li <li.evan.chao@gmail.com> wrote:
> I just reviewed the patch. Overall looks solid to me.

Thanks for the review!

> Putting “…” inside a function body looks quite uncommon. I searched over the source tree and couldn't find other
occurrence.As the comment has explained why omitting pg_neg_size_overflow, maybe just remove the entry #if 0 block, or
justleave an empty function body. 

My intent is just to document what the signature would have been. But
with Michael adding that it could confuse a casual grepper, I think
I'll switch to a standard comment, at minimum.

Thanks,
--Jacob

On Tue, Nov 18, 2025 at 9:18 PM Michael Paquier <michael@paquier.xyz> wrote:
> This is following the same pattern as what has been introduced in
> 7dedfd22b798 for the other unsigned types in int.h.  Anyway, looking
> at that separately, the logic of 0001 seems right here.

Okay, good. Thanks for the review!

> > 2) I have not implemented pg_neg_size_overflow(), because to me it
> > seems likely to be permanently dead code, and it would require
> > additional reasoning about the portability of SSIZE_MAX.
> > (pg_sub_size_overflow(), by contrast, is easy to do and feels like it
> > might be useful to someone eventually.)
>
> Documenting the portability issue is important, indeed.  I'd suggest
> to not use a ifdef 0, though, which may be confusing on grep if one
> does not look at the surrounding lines.  Leaving that in the shape of
> a comment would be hard to miss.

Done with a standard comment in v2, attached. Or were you also
suggesting that I should just get rid of the sample, and rely on the
comment above it?

> Anyway, are you worrying about
> SIZE_MAX matching with something different than the compile-time value
> at runtime?

(To avoid any confusion: I'm referring to SSIZE_MAX in particular,
from POSIX, not SIZE_MAX which is C99.)

My concern is that I'll need to add code to win32_port.h for this,
alongside our existing ssize_t definition, and then use the buildfarm
to flush out collisions with any other third-party headers that might
have done the same on Windows. That seems like a lot of potential pain
for no benefit.

--Jacob

On Wed, Nov 19, 2025 at 9:46 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Done with a standard comment in v2, attached. Or were you also
> suggesting that I should just get rid of the sample, and rely on the
> comment above it?

Pushed v2; let me know if some other documentation style would be more helpful.

Thank you both for the reviews!

--Jacob