query_int wrongly parses zero-padded input

Marko Grujic <marko.grujic@enterprisedb.com> writes:

I'm reporting a minor, long-standing, edge case in intarray here.
Namely, when parsing a query_int, the strtol base is set to 0, which forces
the base auto-detection mode.
Consequently, when a zero-padded integer input is provided, the number is
interpreted as an octal value, which in turn leads to wrong int-query results

Hmm ... of course, "wrong" is in the eye of the beholder. There are a
large number of places where we intentionally use base 0, mostly in
service of allowing hex input. I tend to agree with you that this
case wasn't intentional, since the preceding parsing logic only
accepts a sign and 0-9, not "x" or hex digits. Still, we should
consider whether the right answer is to fix that logic to allow
hex input. A (weak) argument for going that way is we'd stay
bug-compatible with the existing behavior for octal input.

I checked around and couldn't find any other places where it seemed
like base zero was being specified unintentionally: in particular
we intentionally use it for integer GUCs, and the parsing logic
is such that you can put in 0x-whatever in all the related places.

On the whole I agree with you that this is a bug and it should use
base 10, but we should wait to see if anyone disagrees.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Still, we should consider whether the right answer is to fix that logic
to allow hex input. A (weak) argument for going that way is we'd stay
bug-compatible with the existing behavior for octal input.

I ran a differential sweep to put numbers on that choice: unpatched master
(base 0) against master + Marko's patch (base 10), same server binaries,
only _int.so differs, ~264k parsed inputs. Full write-up and repro are
attached (evidence.txt); the parts that bear on the decision:

Hex and 0b are unreachable, measured. gettoken() copies only '0'-'9' and a
leading '-' into the operand buffer, so strtol never sees 'x' or 'b'. On
unpatched master every one of 0x10, 0X1F, 0b1, 0o7 is already a syntax
error. (_int.so here links __isoc23_strtol, so base 0 *would* take 0x and
0b if either reached it -- it doesn't.) So there is no existing hex
behavior to stay compatible with: allowing hex would be adding a feature,
not preserving one. The only thing base 0 does today is read a
leading-zero operand as octal.

And that octal reading is the harmful part, not something worth keeping. A
leading-zero operand containing an 8 or 9 is truncated silently, not
rejected: '08' and '09' both parse as 0, '-08' as 0 with the sign dropped,
'018' as 1, and '02147483647' as 9020. That is worse, and quieter, than
the '010' -> 8 in Marko's report. Every
behavior change between the two builds falls in this leading-zero class
(33,883 of ~264k inputs; none outside it). The change is per operand and
does not touch the operators, NOT, parentheses or nesting (4,007 random
boolean queries, exact match in both directions), and @@/~~ stay
consistent across seqscan, gist__int_ops, gist__intbig_ops and
gin__int_ops (0 disagreements). Output carries no leading zero, so values
already stored in parsed form survive dump/restore unchanged; only query
text parsed again after the upgrade picks up the decimal reading.

One compatibility point worth a line in the commit message: base 10 does
reject a small set of inputs base 0 accepted -- leading-zero values whose
octal reading fits in int32 but whose decimal reading does not, e.g.
'017777777777' (octal INT_MAX) and '-020000000000' (octal INT_MIN); 5,178
such inputs in the sweep. Nothing goes the other way.

So the measurement lines up with your and Marko's read: base 10 is the
fix, and the hex route would be new behavior rather than kept behavior. If
it helps, tests-suggested.diff (attached, on top of 0001) adds the
silent-truncation and now-rejected cases; it passes on the patched build
and fails on master.

regards,
Manu

Manu <manuelreyesbravo@gmail.com> writes:

And that octal reading is the harmful part, not something worth keeping. A
leading-zero operand containing an 8 or 9 is truncated silently, not
rejected: '08' and '09' both parse as 0, '-08' as 0 with the sign dropped,
'018' as 1, and '02147483647' as 9020. That is worse, and quieter, than
the '010' -> 8 in Marko's report.

Oh, that's an excellent point: if we did want to go the direction of
exposing strtol's full behavior, we'd need to make the code check
that strtol ate the whole input, which it does not today. I think
it's unnecessary to add such a check if we switch to base 10
(maybe I'm wrong?).

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Oh, that's an excellent point: if we did want to go the direction of
exposing strtol's full behavior, we'd need to make the code check
that strtol ate the whole input, which it does not today. I think
it's unnecessary to add such a check if we switch to base 10
(maybe I'm wrong?).

I think you're right, and I measured it rather than reason about it. I
instrumented the one strtol() call in _int_bool.c and swept every byte
0x01-0xFF in seven operand positions (SQL_ASCII, so high bytes arrive
unconverted): the only bytes that ever reach the buffer are '-' and '0'-'9',
the same under base 0 and base 10. Everything else -- x, b, o, e, '+', '_',
'.', and all of 0x80-0xFF -- is a syntax error before strtol() is called. So
strtol() is always handed an optional sign followed by decimal digits, and
under base 10 it consumes all of them; there is no trailing input for an "ate
the whole input" check to catch.

To put a number on it, I ran 72,222,250 operand buffers (the short ones
exhaustively, the longer ones sampled) through strtol() with an end-pointer
check. Under base 10 exactly one is not consumed whole: the lone "-", where
strtol() returns 0 and advances nothing, so '1&-' parses as '1 & 0'. Under
base 0, 23,620,854 of them are not -- every leading-zero operand that reaches
an 8 or 9, which is the bug. A cross-check against the instrumented server
matched on all 29,430 sampled buffers (buffer, endptr, value, errno and
output), so this is the server's behavior, not a model's.

Two things so "nothing left to guard" isn't overstated. The existing
errno/int32 range check still earns its keep: an overflowing operand like
'99999999999' is consumed whole and rejected there, not through endptr. And
the lone "-" is pre-existing and base-independent -- base 0 accepts it as 0
today too; an end-pointer check would reject it, but that is a separate
question from the base, so I would not fold it into this patch.

regards,
Manu

Manu <manuelreyesbravo@gmail.com> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

Oh, that's an excellent point: if we did want to go the direction of
exposing strtol's full behavior, we'd need to make the code check
that strtol ate the whole input, which it does not today. I think
it's unnecessary to add such a check if we switch to base 10
(maybe I'm wrong?).

I think you're right, and I measured it rather than reason about it.

Really? Maybe you proved something about the specific strtol
implementation you tested, but it's a wide world out there.

The thing I was wondering about was whether an implementation
might stop short of reading the whole digit string once it
realizes it's reached overflow. POSIX seems to require the
endptr to be placed after the last digit --- but if we don't
ask for an endptr value, the library would be well within its
rights to not scan all the way to the end. However, that's
moot since we check both for errno being set and for an
out-of-int32-range result, both of which should get tripped
if the library does quit early on overflow.

Now that I'm looking at the POSIX spec, there's also this
fun tidbit:

In other than the C or POSIX locale, additional locale-specific
subject sequence forms may be accepted.

The Linux/glibc man page mentions this but does not indicate that
glibc actually implements any such thing; but perhaps other platforms
do. I think this provision is moot as long as the input can only
contain a sign followed by ASCII digits. But if we accepted other
characters, that could be another route to strtol deciding that the
string is acceptable but has trailing junk.

In short, I concur that we shouldn't need a trailing-junk check
given the current parsing behavior; but I think the safe way to
arrive at that conclusion is by reading the POSIX standard
not by exhaustively testing one specific version of one
specific library.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

The thing I was wondering about was whether an implementation might stop
short of reading the whole digit string once it realizes it's reached
overflow. [...] that's moot since we check both for errno being set and
for an out-of-int32-range result, both of which should get tripped if the
library does quit early on overflow.

Point taken on the method; reasoning from the code rather than the test, the
two checks turn out to be load-bearing on different platforms, which is why
neither alone would do. gettoken() caps the operand at 15 characters
(nnn[16]), so the value handed to strtol() is at most ~10^15. Where long is
64-bit, that is far below LONG_MAX, so strtol() cannot overflow and errno is
never set on this input: an out-of-int32 value is read in full -- no early
stop to worry about -- and is rejected only by the explicit
"(long) *val != lval" round-trip. Where long is 32-bit, an out-of-int32
operand does overflow strtol(), and POSIX requires ERANGE, which the errno
check catches. So the "quit early on overflow" case is confined to platforms
with a 32-bit long, and ERANGE covers it there; on a 64-bit long it can't
arise. Either way no trailing-junk check is needed.

regards,
Manu

On Oct 18, 2026, at 10:54, Manu <manuelreyesbravo@gmail.com> wrote:

Tom Lane <tgl@sss.pgh.pa.us> writes:

The thing I was wondering about was whether an implementation might stop
short of reading the whole digit string once it realizes it's reached
overflow. [...] that's moot since we check both for errno being set and
for an out-of-int32-range result, both of which should get tripped if the
library does quit early on overflow.

Point taken on the method; reasoning from the code rather than the test, the
two checks turn out to be load-bearing on different platforms, which is why
neither alone would do. gettoken() caps the operand at 15 characters
(nnn[16]), so the value handed to strtol() is at most ~10^15. Where long is
64-bit, that is far below LONG_MAX, so strtol() cannot overflow and errno is
never set on this input: an out-of-int32 value is read in full -- no early
stop to worry about -- and is rejected only by the explicit
"(long) *val != lval" round-trip. Where long is 32-bit, an out-of-int32
operand does overflow strtol(), and POSIX requires ERANGE, which the errno
check catches. So the "quit early on overflow" case is confined to platforms
with a 32-bit long, and ERANGE covers it there; on a 64-bit long it can't
arise. Either way no trailing-junk check is needed.

regards,
Manu

The issue was introduced by 7ccb6dc2d3e, which switched atoi() to strtol(). Since atoi() uses base 10, changing strtol() to use base 10 restores the original decimal interpretation. That seems reasonable.

The variable "nnn" may contain an optional leading "-" sign followed by digits "0-9", so with base 10, conversion should consume the whole operand if it contains at least one digit. Overflow is handled by the existing errno and "(long) *val != lval" checks. This part also seems fine.

The only thing I see is the handling of a bare "-" sign. I get an error on my MacBook:
```
evantest=# select '-'::query_int;
LINE 1: select '-'::query_int;
^
ERROR: syntax error
LINE 1: select '-'::query_int;
^

evantest=# select '1&-'::query_int;
LINE 1: select '1&-'::query_int;
^
ERROR: syntax error
LINE 1: select '1&-'::query_int;
^
```

But this relies on strtol() setting errno. Reading strtol(3p) at [1]https://man7.org/linux/man-pages/man3/strtol.3p.html, which is the POSIX Programmer's Manual, the relevant wording is:
```
Upon successful completion, these functions shall return the
converted value, if any. If no conversion could be performed, 0
shall be returned and errno may be set to [EINVAL].
```

It says 0 SHALL be returned, but errno MAY be set to EINVAL, which seems to imply that implementations are not required to set errno for a bare "-", which explains the difference between my result and Manu’s. I confirmed in the debugger that strtol("-") sets errno to EINVAL on my MacBook.

So, based on the POSIX specification, we would need an end-pointer check to ensure that a bare "-" consistently raises an error.

[1]: https://man7.org/linux/man-pages/man3/strtol.3p.html

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

Fair point about the bare minus sign, and the man pages for strtol(3p) seem
to provide a neat way to handle it:

If the subject sequence is empty or does not have the expected
form, no conversion is performed; the value of nptr shall be
stored in the object pointed to by endptr, provided that endptr is
not a null pointer.

So it seems like if pass the 2nd arg (endptr), and it turns out to be
something other than the NUL byte,
the parsing failed and we should return the ERR code (attached v2 now does
that).

Thanks,
Marko

On Mon, Oct 19, 2026 at 7:39 AM Chao Li <li.evan.chao@gmail.com> wrote:

On Oct 19, 2026, at 16:30, Marko Grujic <marko.grujic@enterprisedb.com> wrote:

Fair point about the bare minus sign, and the man pages for strtol(3p) seem to provide a neat way to handle it:

If the subject sequence is empty or does not have the expected
form, no conversion is performed; the value of nptr shall be
stored in the object pointed to by endptr, provided that endptr is
not a null pointer.

So it seems like if pass the 2nd arg (endptr), and it turns out to be something other than the NUL byte,
the parsing failed and we should return the ERR code (attached v2 now does that).

Thanks,
Marko

On Mon, Oct 19, 2026 at 7:39 AM Chao Li <li.evan.chao@gmail.com> wrote:

On Oct 18, 2026, at 10:54, Manu <manuelreyesbravo@gmail.com> wrote:

Tom Lane <tgl@sss.pgh.pa.us> writes:

The thing I was wondering about was whether an implementation might stop
short of reading the whole digit string once it realizes it's reached
overflow. [...] that's moot since we check both for errno being set and
for an out-of-int32-range result, both of which should get tripped if the
library does quit early on overflow.

Point taken on the method; reasoning from the code rather than the test, the
two checks turn out to be load-bearing on different platforms, which is why
neither alone would do. gettoken() caps the operand at 15 characters
(nnn[16]), so the value handed to strtol() is at most ~10^15. Where long is
64-bit, that is far below LONG_MAX, so strtol() cannot overflow and errno is
never set on this input: an out-of-int32 value is read in full -- no early
stop to worry about -- and is rejected only by the explicit
"(long) *val != lval" round-trip. Where long is 32-bit, an out-of-int32
operand does overflow strtol(), and POSIX requires ERANGE, which the errno
check catches. So the "quit early on overflow" case is confined to platforms
with a 32-bit long, and ERANGE covers it there; on a 64-bit long it can't
arise. Either way no trailing-junk check is needed.

<v2-0001-intarray-Parse-query_int-integers-as-decimals.patch>

Would "endptr == nnn" be more direct here? That checks whether no conversion occurred, as described in the quoted doc. The check in v2 also works because nnn cannot be empty, so this is just a preference.

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

Would "endptr == nnn" be more direct here? That checks whether no

conversion occurred, as described in the quoted doc.

Agreed, that would be more direct, with one caveat (which
admittedly doesn't apply today),
and that is that the endptr-not-at-NUL-byte variant also safe-guards
against trailing-junk (which Tom mentioned).

Thanks,
Marko

On Mon, Oct 19, 2026 at 10:52 AM Chao Li <li.evan.chao@gmail.com> wrote: