Subject: oauth integer overflow

From: Andres Freund <[email-masked]>
Date: Thu, 23 Apr 2026 12:12:37 -0400

Hi,

I was once more looking at what it'd take to work with -ftrapv, when cassert
is enabled.  Partially motivated with the worry that we support compilers that
don't understand -fwrapv so code relying on signed overflow isn't actually
safe. And because it sometimes leads to unexpected results that can cause
trouble and -ftrapv helps find those.

One thing that quickly triggers when doing so is:

#0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
#1  0x00007fe1a05a9dbf in __pthread_kill_internal (threadid=<optimized out>, signo=6) at ./nptl/pthread_kill.c:89
#2  0x00007fe1a0552d02 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x00007fe1a053a4b2 in __GI_abort () at ./stdlib/abort.c:77
#4  0x00007fe19f218ac1 in __addvsi3 ()
   from /srv/dev/build/postgres/m-dev-assert/tmp_install//srv/dev/install/postgres/m-dev-assert/lib/x86_64-linux-gnu/libpq-oauth.so
#5  0x00007fe19f20da05 in handle_token_response (actx=0x561825a06350, token=0x7ffc2f677e48)
    at ../../../../../home/andres/src/postgresql/src/interfaces/libpq-oauth/oauth-curl.c:2625
#6  0x00007fe19f20dec5 in pg_fe_run_oauth_flow_impl (conn=0x561825998490, request=0x561825a06e30, altsock=0x561825998860)
    at ../../../../../home/andres/src/postgresql/src/interfaces/libpq-oauth/oauth-curl.c:2924
#7  0x00007fe19f20e0a8 in pg_fe_run_oauth_flow (conn=0x561825998490, request=0x561825a06e30, altsock=0x561825998860)
    at ../../../../../home/andres/src/postgresql/src/interfaces/libpq-oauth/oauth-curl.c:3027
#8  0x00007fe1a0a35627 in do_async (state=0x5618259a1880, request=0x561825a06e30)
    at ../../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-auth-oauth.c:1528
#9  0x00007fe1a0a346e3 in run_oauth_flow (conn=0x561825998490) at ../../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-auth-oauth.c:751
#10 0x00007fe1a0a3ffc1 in PQconnectPoll (conn=0x561825998490) at ../../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-connect.c:4302
#11 0x00007fe1a0a3db6c in pqConnectDBComplete (conn=0x561825998490) at ../../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-connect.c:2893
#12 0x00007fe1a0a3a1f7 in PQconnectdbParams (keywords=0x5618259983f0, values=0x561825998440, expand_dbname=1)

	/*
	 * A slow_down error requires us to permanently increase our retry
	 * interval by five seconds.
	 */
	if (strcmp(err->error, "slow_down") == 0)
	{
		int			prev_interval = actx->authz.interval;

		actx->authz.interval += 5;
		if (actx->authz.interval < prev_interval)
		{
			actx_error(actx, "slow_down interval overflow");
			goto token_cleanup;
		}
	}

I don't think it's safe to rely on that type of check working without -fwrapv
support, which in turn we can't rely on having.

I think this should use pg_add_s32_overflow().

Greetings,

Andres Freund

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 23 Apr 2026 19:28:43 +0200

> On 23 Apr 2026, at 18:12, Andres Freund <[email-masked]> wrote:

> I think this should use pg_add_s32_overflow().

Agreed, I'll write up a patch to fix it.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Thu, 23 Apr 2026 10:49:42 -0700

> > I think this should use pg_add_s32_overflow().

Agreed, thanks for the report!

> Agreed, I'll write up a patch to fix it.

Cool. I have one written up and can share it for comparison, if you'd
like, but it's fairly verbose and I wonder if there's a better way to
do it.

--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 23 Apr 2026 20:17:03 +0200

> On 23 Apr 2026, at 19:49, Jacob Champion <[email-masked]> wrote:

> like, but it's fairly verbose and I wonder if there's a better way to

Well, if you're already done then please do share it, and we'll use that as a
starting point.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Thu, 23 Apr 2026 11:31:34 -0700

> Well, if you're already done then please do share it, and we'll use that as a

Attached. The static_assert for the millisecond calculation is the
only part I don't really like, but doing an overflow check on a
calculation that can't overflow int64 is even more verbose/wasteful.

--Jacob

---

From: Andres Freund <[email-masked]>
Date: Thu, 23 Apr 2026 14:37:26 -0400

Hi,

> calculation that can't overflow int64 is even more verbose/wasteful.

How about instead making sure that actx->authz.interval never gets big enough
to have any chance of overflowing during either the += 5 or the * 1000?  It's
clearly ok to error out well before that...

Greetings,

Andres Freund

---

From: Jacob Champion <[email-masked]>
Date: Thu, 23 Apr 2026 12:05:32 -0700

> clearly ok to error out well before that...

It probably is, but I guess the approach depends on whether you prefer
checking at the time of operation, or attempting to reason about it
ahead of time in far-away code. With the latter, if additional math is
added in the future, then either the new overflow hazard gets missed,
or the ceiling gets lowered again, or the new math gets an overflow
check when the others don't. I prefer the time-of-use pattern,
personally.

--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 28 Apr 2026 13:17:54 +0200

> On 23 Apr 2026, at 21:05, Jacob Champion <[email-masked]> wrote:
> 

I am fine with your approach in the attached patch.  If you don't like the
static assert you could move it to be out of the way, and expand the comment
for it to what it means if it hits.  Just one small nitpick on the patch:

+	 * LONG_MAX milliseconds is 24 days on 32-bit platforms,
+	 * which for most people is going to be equivalent to a
+	 * disabled timer... but avoid overflow in case the

When teading "disabled timer" I interpret that as a timer which is 0 and has no
interval (which might be due to not being a native speaker), but what it
actually describes is an interval which (in practice) never ends.  Perhaps it
could be phrased more like "for most people is going to be equivalent to a
never ending interval".

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Fri, 5 Jun 2026 12:06:27 -0700

> calculation that can't overflow int64 is even more verbose/wasteful.

I was preparing to commit this for beta1 last week, and I realized
that I've changed my tune. With all the recent backports of overflow
checks, I think I need to be reaching for them by default, especially
in a non-performance-critical path.

v2 rewrites that part with a checked multiplication, which removes any
need for a static_assert complication.

> could be phrased more like "for most people is going to be equivalent to a

This has been completely rewritten now; see what you think.

Thanks!
--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 5 Jun 2026 21:25:27 +0200

> On 5 Jun 2026, at 21:06, Jacob Champion <[email-masked]> wrote:
> 

I agree with this approach, +1 on this version.

> This has been completely rewritten now; see what you think.

LGTM.

--
Daniel Gustafsson