Fix a host of strto*() bugs

On Thu, Jul 30, 2026 at 08:55:02AM +0000, Tristan Partin wrote:

In bug #19584[0], the user reported that tid parsing was platform dependent
due to the way that strtoul() works. Take the following code:

strtoul("", &endptr, 10);

On glibc, the return value is 0, errno is not set, and endptr points to
the empty string. On Apple libc, the return value is 0, errno is set to
EINVAL, and endptr points to the empty string.

@@ -961,7 +961,7 @@ precheck_tar_backup_file(verifier_context *context, char *relpath,
          * Report an error if we didn't consume at least one character, if the
          * result is 0, or if the value is too large to be a valid OID.
          */
-        if (suffix == NULL || num <= 0 || num > OID_MAX)
+        if (suffix == relpath || num <= 0 || num > OID_MAX)

Because that's something part of the C standard. My Linux man page
has a reference to that, as well:
"If there were no digits at all, strtoul() stores the original value
of nptr in *endptr (and returns 0)."

The tid parsing code was correctly handling EINVAL, but did not handle
the case where endptr == input, which we do in quite a few places around
the codebase. The end effect was that glibc systems accepted "(1,)" and
"(,1)" as valid tids for parsing purposes, while Apple libc systems did
not.

It looks like there is a formatting problem with your patch 0001. It
fails to apply here, and some chunks seem misplaced. 0004 is also
showing a similar problem.

The first patch in the series fixes this issue. I did some analysis on
other uses of strtoul() and friends and found a few more places where
integers were not being correctly parsed from strings. Those are each
attached as individual patches to ease backpatching if it is determined
that we should. Otherwise, I suggest squashing the series. I wonder if
we should come up with a helper macro for helping callers handle errors
correctly? Or should we just always check this case?

Some of it could be perhaps unified, but there is also some beauty in
letting the callers handle things on their own. strtou64() is for
example just a macro that stands on top of strtoul[l]().

The tests for proto_version feel expensive for the coverage brought.
I'm finding the ones for 005_bad_manifest.pl actually acceptable to
have for the three cases patched. No actual comment for
pg_verifybackup.c, removing the NULL check is well, right.

Regarding 0001, yes, that's a bug. It is definitely not something
that we could backpatch as we'd begin to reject inputs that were
accidentally rejected, even if I am hoping that nobody in their right
mind would use a '(1,)'::tid or a '(,1)'::tid to map to respectively
(1,0) or (0,1).

Something similar could be said about 0004: bug, no backpatch. Now
it's not the most beautiful piece of software now sitting in the
tree..

Could you fix 0001 and 0004 please? I'll see about applying some of
the pieces you have here. 0002, 0003 and 0005 are no-brainers, but
I'd tend to remove the rather expensive tests of 0003 in the end
result, the coverage vs runtime cost is not appealing.
--
Michael

On Fri, Jul 31, 2026 at 05:03:38PM +0900, Michael Paquier wrote:

@@ -961,7 +961,7 @@ precheck_tar_backup_file(verifier_context *context, char *relpath,
* Report an error if we didn't consume at least one character, if the
* result is 0, or if the value is too large to be a valid OID.
*/
-        if (suffix == NULL || num <= 0 || num > OID_MAX)
+        if (suffix == relpath || num <= 0 || num > OID_MAX)

Because that's something part of the C standard. My Linux man page
has a reference to that, as well:
"If there were no digits at all, strtoul() stores the original value
of nptr in *endptr (and returns 0)."

0002 and 0005 have been merged together, and applied as 355814931141.
--
Michael

On Fri, Jul 31, 2026 at 05:03:38PM +0900, Michael Paquier wrote:

Regarding 0001, yes, that's a bug. It is definitely not something
that we could backpatch as we'd begin to reject inputs that were
accidentally rejected, even if I am hoping that nobody in their right
mind would use a '(1,)'::tid or a '(,1)'::tid to map to respectively
(1,0) or (0,1).

One question that I have been asking to myself here is: could the ODBC
driver internally generate a tid value that may not be cross-platform
portable? And after diving into the code I think that the answer is
no by itself, but there are paths where a client application can
provide its own value. So I think that there is no practical argument
against making the strtoul() call with tid parsing more compliant to
the C standard. At least let's try it so on HEAD. If that proves to
be a experiment failure, we could always revert during the beta cycle
and move back to the old behavior. Here I'd favor more consistency in
our code base.
--
Michael

On Sun Aug 2, 2026 at 10:26 AM UTC, Michael Paquier wrote:

On Fri, Jul 31, 2026 at 05:03:38PM +0900, Michael Paquier wrote:

@@ -961,7 +961,7 @@ precheck_tar_backup_file(verifier_context *context, char *relpath,
* Report an error if we didn't consume at least one character, if the
* result is 0, or if the value is too large to be a valid OID.
*/
-        if (suffix == NULL || num <= 0 || num > OID_MAX)
+        if (suffix == relpath || num <= 0 || num > OID_MAX)

Because that's something part of the C standard. My Linux man page
has a reference to that, as well:
"If there were no digits at all, strtoul() stores the original value
of nptr in *endptr (and returns 0)."

0002 and 0005 have been merged together, and applied as 355814931141.

Here are some re-spins of the previous uncommitted patches. They have
been rebased on master. I am really curious how the formatting got
messed up initially. Thanks for committing the the two patches.

--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)

On Mon, Aug 03, 2026 at 08:32:38PM +0000, Tristan Partin wrote:

Here are some re-spins of the previous uncommitted patches. They have
been rebased on master. I am really curious how the formatting got
messed up initially. Thanks for committing the the two patches.

It looks to me that you have missed the main material of the
discussion: the patches themselves.

:p
--
Michael

On Mon Aug 3, 2026 at 10:08 PM UTC, Michael Paquier wrote:

On Mon, Aug 03, 2026 at 08:32:38PM +0000, Tristan Partin wrote:

Here are some re-spins of the previous uncommitted patches. They have
been rebased on master. I am really curious how the formatting got
messed up initially. Thanks for committing the the two patches.

It looks to me that you have missed the main material of the
discussion: the patches themselves.

Uhh... check this email...

--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)

On 2026-Jul-31, Michael Paquier wrote:

On Thu, Jul 30, 2026 at 08:55:02AM +0000, Tristan Partin wrote:

The first patch in the series fixes this issue. I did some analysis on
other uses of strtoul() and friends and found a few more places where
integers were not being correctly parsed from strings. Those are each
attached as individual patches to ease backpatching if it is determined
that we should. Otherwise, I suggest squashing the series. I wonder if
we should come up with a helper macro for helping callers handle errors
correctly? Or should we just always check this case?

Some of it could be perhaps unified, but there is also some beauty in
letting the callers handle things on their own. strtou64() is for
example just a macro that stands on top of strtoul[l]().

Maybe we should add our wrapper with more consistent error reporting
behavior that hides all those platform-dependent differences and uses an
error reporting mechanism that doesn't rely on errno. I think that
would result in simpler and more consistent code.

I'm thinking something like
uint64 pg_strtouint64(const char *s, char **endptr, uint32 options, error_callback cb);

where we parse and return the number, and in case of any failure, we
call the error callback which can do an ereport() or pg_fatal() or
whatever. So the caller need only set up a callback, and then all calls
to the conversion function can be straightforward and not concern
themselves with the platform specific errno handling.

We can use the options bitmask to mediate any potential caller-specific
needs, for example
INTEGER_CONV_OCTAL
INTEGER_CONV_HEX
to override the default assumption of base 10.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Nunca confiaré en un traidor. Ni siquiera si el traidor lo he creado yo"
(Barón Vladimir Harkonnen)

On Tue, Aug 04, 2026 at 05:14:43PM +0200, Alvaro Herrera wrote:

I'm thinking something like
uint64 pg_strtouint64(const char *s, char **endptr, uint32 options, error_callback cb);

where we parse and return the number, and in case of any failure, we
call the error callback which can do an ereport() or pg_fatal() or
whatever. So the caller need only set up a callback, and then all calls
to the conversion function can be straightforward and not concern
themselves with the platform specific errno handling.

We can use the options bitmask to mediate any potential caller-specific
needs, for example
INTEGER_CONV_OCTAL
INTEGER_CONV_HEX
to override the default assumption of base 10.

I am not sure that we would need to diverge from the existing "base"
argument, passing down an option which is an equivalent of it feels
kind of confusing to me.

While looking at all the callers of strtou64() and strtoi64(), I can't
help but notice that each call has its own assumptions in terms of
endptr handling, minus the range checks they may or may not do. An
error callback may be adapted if some of the range checks are fixed,
but some of them rely on a parse state (parse_manifest.c). With all
that in mind, I am not entirely convinced that this would lead to an
overall simplification, quite the opposite actually. The new pg_()
routine could be given a context pointer that is passed down as an
argument of the error_callback, still that doesn't stick well here..
--
Michael

On Mon, Aug 03, 2026 at 10:40:21PM +0000, Tristan Partin wrote:

Uhh... check this email...

I am not sure what is wrong, if that's on your side or mine, but your
patch for the tid bits was not able to apply because your generated
patch included twice the same diffs. Anyway, I got something out and
applied the tid part as of 58ff4a0a0867, and the pgoutput part as of
290bb8afe6b4.

For the pgoutput thing, I have added a much cheaper test than what you
proposed, in the shape of a single SQL query in test_decoding.

Patch 0003 for ecpg has the same set of issues as 0001: duplicated
blocks of diffs. I have not looked at it yet. Perhaps later.
--
Michael

On Thu Aug 6, 2026 at 8:09 PM CDT, Michael Paquier wrote:

On Mon, Aug 03, 2026 at 10:40:21PM +0000, Tristan Partin wrote:

Uhh... check this email...

I am not sure what is wrong, if that's on your side or mine, but your
patch for the tid bits was not able to apply because your generated
patch included twice the same diffs. Anyway, I got something out and
applied the tid part as of 58ff4a0a0867, and the pgoutput part as of
290bb8afe6b4.

For the pgoutput thing, I have added a much cheaper test than what you
proposed, in the shape of a single SQL query in test_decoding.

Patch 0003 for ecpg has the same set of issues as 0001: duplicated
blocks of diffs. I have not looked at it yet. Perhaps later.

So strange. I am not doing anything special. Just a git-format-patch.
Let me investigate more later today.

--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)

On Fri, Aug 07, 2026 at 04:02:52PM +0000, Tristan Partin wrote:

So strange. I am not doing anything special. Just a git-format-patch.
Let me investigate more later today.

Could you refresh the ecpg patch please?
--
Michael

On Thu Aug 13, 2026 at 6:57 AM UTC, Michael Paquier wrote:

On Fri, Aug 07, 2026 at 04:02:52PM +0000, Tristan Partin wrote:

So strange. I am not doing anything special. Just a git-format-patch.
Let me investigate more later today.

Could you refresh the ecpg patch please?

I did find an issue in the v2 of this patch. I tested the new v3 against
master and now it cleanly applies.

--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)