Re: 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