Strange interval arithmetic

What's going on here? Some sort of integer wraparound?

WORKS
=====

mysql=# select interval '2378 seconds';
interval
----------
00:39:38
(1 row)

mysql=#
mysql=# select 2378 * interval '1 second';
?column?
----------
00:39:38
(1 row)

DOESN'T WORK
============

test=# select interval '2378234234 seconds';
interval
--------------
596523:14:07
(1 row)

test=# select 2378234234 * interval '1 second';
?column?
--------------
660620:37:14
(1 row)

On Sun, Nov 27, 2005 at 11:15:04PM +0800, Christopher Kings-Lynne wrote:
> What's going on here? Some sort of integer wraparound?
[...]
> test=# select interval '2378234234 seconds';
> interval
> --------------
> 596523:14:07
> (1 row)

Looks like the value is stuck at 2^31 - 1 seconds:

test=> select interval '2147483646 seconds'; -- 2^31 - 2
interval
--------------
596523:14:06
(1 row)

test=> select interval '2147483647 seconds'; -- 2^31 - 1
interval
--------------
596523:14:07
(1 row)

test=> select interval '2147483648 seconds'; -- 2^31
interval
--------------
596523:14:07
(1 row)

--
Michael Fuhr

On Sun, Nov 27, 2005 at 08:45:18AM -0700, Michael Fuhr wrote:
> Looks like the value is stuck at 2^31 - 1 seconds:

I see this behavior back to at least 7.3. I'd guess it's because
strtol() indicates overflow by returning LONG_MAX and setting errno
to ERANGE, but the code doesn't check for that.

--
Michael Fuhr

On Sun, Nov 27, 2005 at 11:27:54AM -0700, Michael Fuhr wrote:
> On Sun, Nov 27, 2005 at 08:45:18AM -0700, Michael Fuhr wrote:
> > Looks like the value is stuck at 2^31 - 1 seconds:
>
> I see this behavior back to at least 7.3. I'd guess it's because
> strtol() indicates overflow by returning LONG_MAX and setting errno
> to ERANGE, but the code doesn't check for that.

Is this worth looking at for the upcoming dot releases? It's
apparently a longstanding behavior that almost nobody encounters,
yet knowingly not addressing it seems a bit MySQLish ;-) Here's
the start of the thread for anybody who missed it:

http://archives.postgresql.org/pgsql-hackers/2005-11/msg01385.php

--
Michael Fuhr

Michael Fuhr <mike(at)fuhr(dot)org> writes:
>> I see this behavior back to at least 7.3. I'd guess it's because
>> strtol() indicates overflow by returning LONG_MAX and setting errno
>> to ERANGE, but the code doesn't check for that.

> Is this worth looking at for the upcoming dot releases?

Sure, send a patch ...

regards, tom lane

On Wed, Nov 30, 2005 at 12:37:40PM -0500, Tom Lane wrote:
> Michael Fuhr <mike(at)fuhr(dot)org> writes:
> >> I see this behavior back to at least 7.3. I'd guess it's because
> >> strtol() indicates overflow by returning LONG_MAX and setting errno
> >> to ERANGE, but the code doesn't check for that.
>
> > Is this worth looking at for the upcoming dot releases?
>
> Sure, send a patch ...

Any preferences on an approach? The simplest and easiest to verify
would be to raise an error for just this particular case; a TODO
item might be to change how the string is parsed to allow values
larger than LONG_MAX. I see several calls to strtol() that aren't
checked for overflow but that might not be relevant to this problem,
so I'm thinking this patch ought not touch them. Maybe that's another
TODO item.

--
Michael Fuhr

Michael Fuhr <mike(at)fuhr(dot)org> writes:
> On Wed, Nov 30, 2005 at 12:37:40PM -0500, Tom Lane wrote:
>> Sure, send a patch ...

> Any preferences on an approach? The simplest and easiest to verify
> would be to raise an error for just this particular case; a TODO
> item might be to change how the string is parsed to allow values
> larger than LONG_MAX.

I think the latter would be a feature enhancement and therefore not
good material to back-patch. Just erroring out seems appropriate
for now.

> I see several calls to strtol() that aren't checked for overflow but
> that might not be relevant to this problem, so I'm thinking this patch
> ought not touch them. Maybe that's another TODO item.

If it's possible for them to be given overflowing input, they probably
ought to be checked.

regards, tom lane

On Wed, Nov 30, 2005 at 02:01:46PM -0500, Tom Lane wrote:
> Michael Fuhr <mike(at)fuhr(dot)org> writes:
> > Any preferences on an approach? The simplest and easiest to verify
> > would be to raise an error for just this particular case; a TODO
> > item might be to change how the string is parsed to allow values
> > larger than LONG_MAX.
>
> I think the latter would be a feature enhancement and therefore not
> good material to back-patch. Just erroring out seems appropriate
> for now.

Agreed. I'm thinking about rewriting strtol() calls in datetime.c
to look like this:

errno = 0;
val = strtol(field[i], &cp, 10);
if (errno == ERANGE)
return DTERR_FIELD_OVERFLOW;

Does that look okay? Or would you rather raise an error with ereport()?

> > I see several calls to strtol() that aren't checked for overflow but
> > that might not be relevant to this problem, so I'm thinking this patch
> > ought not touch them. Maybe that's another TODO item.
>
> If it's possible for them to be given overflowing input, they probably
> ought to be checked.

I'm looking at all the strtol() calls in datetime.c right now; I
haven't looked anywhere else yet. Should I bother checking values
that will be range checked later anyway? Time zone displacements,
for example?

--
Michael Fuhr

Michael Fuhr wrote:

> Agreed. I'm thinking about rewriting strtol() calls in datetime.c
> to look like this:
>
> errno = 0;
> val = strtol(field[i], &cp, 10);
> if (errno == ERANGE)
> return DTERR_FIELD_OVERFLOW;

Hmm, why not check both the return value _and_ errno:

val = strtol(field[i], &cp, 10);
if (val == LONG_MAX && errno == ERANGE)
return DTERR_FIELD_OVERFLOW;

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Wed, Nov 30, 2005 at 07:06:42PM -0300, Alvaro Herrera wrote:
> Hmm, why not check both the return value _and_ errno:
>
> val = strtol(field[i], &cp, 10);
> if (val == LONG_MAX && errno == ERANGE)
> return DTERR_FIELD_OVERFLOW;

I usually check both in my own code but I noticed several places
where PostgreSQL doesn't, so I kept that style. I'll check both
if that's preferred.

--
Michael Fuhr

Michael Fuhr <mike(at)fuhr(dot)org> writes:
> errno = 0;
> val = strtol(field[i], &cp, 10);
> if (errno == ERANGE)
> return DTERR_FIELD_OVERFLOW;

> Does that look okay? Or would you rather raise an error with ereport()?

Looks fine to me, at least in the routines that are for datetime stuff.

> I'm looking at all the strtol() calls in datetime.c right now; I
> haven't looked anywhere else yet. Should I bother checking values
> that will be range checked later anyway? Time zone displacements,
> for example?

Good question. Is strtol guaranteed to return INT_MAX or INT_MIN on
overflow, or might it return the overflowed value?

regards, tom lane

Michael Fuhr <mike(at)fuhr(dot)org> writes:
> I usually check both in my own code but I noticed several places
> where PostgreSQL doesn't, so I kept that style. I'll check both
> if that's preferred.

I'd say not --- it's more code and it makes a possibly unwarranted
assumption about strtol's behavior.

regards, tom lane

On Wed, Nov 30, 2005 at 05:20:54PM -0500, Tom Lane wrote:
> Michael Fuhr <mike(at)fuhr(dot)org> writes:
> > I'm looking at all the strtol() calls in datetime.c right now; I
> > haven't looked anywhere else yet. Should I bother checking values
> > that will be range checked later anyway? Time zone displacements,
> > for example?
>
> Good question. Is strtol guaranteed to return INT_MAX or INT_MIN on
> overflow, or might it return the overflowed value?

The Open Group Base Specifications say this:

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].

If the correct value is outside the range of representable values,
{LONG_MIN}, {LONG_MAX}, {LLONG_MIN}, or {LLONG_MAX} shall be returned
(according to the sign of the value), and errno set to [ERANGE].

http://www.opengroup.org/onlinepubs/009695399/functions/strtol.html

FreeBSD and Solaris both peg overflow at LONG_MAX, and that behavior
is what I noticed in the first place. I don't know if any systems
behave otherwise. Alvaro suggested checking for both LONG_MAX and
ERANGE; I suppose if we check for LONG_MAX then we should also check
for LONG_MIN. I don't know if any systems might set ERANGE in a
non-error situation.

--
Michael Fuhr

Michael Fuhr <mike(at)fuhr(dot)org> writes:
> I suppose if we check for LONG_MAX then we should also check
> for LONG_MIN.

s/should/must/, which makes the code even more complicated, in order to
buy what exactly?

> I don't know if any systems might set ERANGE in a non-error situation.

The SUS saith
http://www.opengroup.org/onlinepubs/007908799/xsh/strtol.html

The strtol() function will not change the setting of errno if
successful.

Perhaps more to the point, we've been doing it that way (errno test
only) for many years without complaints. Adding a test on the return
value is venturing into less charted waters.

regards, tom lane

On Wed, Nov 30, 2005 at 05:23:23PM -0500, Tom Lane wrote:
> Michael Fuhr <mike(at)fuhr(dot)org> writes:
> > I usually check both in my own code but I noticed several places
> > where PostgreSQL doesn't, so I kept that style. I'll check both
> > if that's preferred.
>
> I'd say not --- it's more code and it makes a possibly unwarranted
> assumption about strtol's behavior.

OTOH, it might be an unwarranted assumption that ERANGE alone
indicates error. It's possible that on some system errno's value
is meaningless unless strtol() returns one of the documented
indicators (LONG_MAX or LONG_MIN). I've seen system calls that
behave that way: errno might get set in a non-error situation due
to the underlying implementation (e.g., wrappers around socket
functions in userland thread implementations), but the programmer
has no business looking at errno unless the function returns -1.

--
Michael Fuhr

On Wed, Nov 30, 2005 at 05:49:53PM -0500, Tom Lane wrote:
> The SUS saith
> http://www.opengroup.org/onlinepubs/007908799/xsh/strtol.html
>
> The strtol() function will not change the setting of errno if
> successful.
>
> Perhaps more to the point, we've been doing it that way (errno test
> only) for many years without complaints. Adding a test on the return
> value is venturing into less charted waters.

Good, I'll stick with just the ERANGE check then.

--
Michael Fuhr

Tom Lane wrote:

>Michael Fuhr <mike(at)fuhr(dot)org> writes:
>
>
>>I suppose if we check for LONG_MAX then we should also check
>>for LONG_MIN.
>>
>>
>
>s/should/must/, which makes the code even more complicated, in order to
>buy what exactly?
>
>
>
>>I don't know if any systems might set ERANGE in a non-error situation.
>>
>>
>
>The SUS saith
>http://www.opengroup.org/onlinepubs/007908799/xsh/strtol.html
>
> The strtol() function will not change the setting of errno if
> successful.
>
>Perhaps more to the point, we've been doing it that way (errno test
>only) for many years without complaints. Adding a test on the return
>value is venturing into less charted waters.
>
>
>
>

LONG_MIN/LONG_MAX might be the actual values provided, too, mightn't
they? checking for ERANGE seems like the only viable test.

cheers

andrew

On Wed, Nov 30, 2005 at 06:00:07PM -0500, Andrew Dunstan wrote:
> LONG_MIN/LONG_MAX might be the actual values provided, too, mightn't
> they? checking for ERANGE seems like the only viable test.

Errno needs to be checked in any case for just that reason; the
question was whether checking *only* errno is sufficient to detect
an error. According to the standard it is.

--
Michael Fuhr

Hmmm...is this something else that needs fixing? The doc says dates
range from 4713 BC to 32767 AD.

test=> select '11754179-08-04'::date;
date
----------------
11754179-08-04
(1 row)

test=> select '11754179-08-05'::date;
date
---------------
4801-01-01 BC
(1 row)

--
Michael Fuhr

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Michael Fuhr <mike(at)fuhr(dot)org> writes:
> > I usually check both in my own code but I noticed several places
> > where PostgreSQL doesn't, so I kept that style. I'll check both
> > if that's preferred.
>
> I'd say not --- it's more code and it makes a possibly unwarranted
> assumption about strtol's behavior.
>

Generally speaking looking at errno when you haven't received an error return
from a libc function is asking for trouble. It could be leftover from any
previous libc error.

That's how you get programs saying things like "strtol: No such file or
directory" ...

The strtol() function returns the result of the conversion, unless the value
would underflow or overflow. If an underflow occurs, strtol() returns
LONG_MIN. If an overflow occurs, strtol() returns LONG_MAX. In both cases,
errno is set to ERANGE. Precisely the same holds for strtoll() (with LLONG_MIN
and LLONG_MAX instead of LONG_MIN and LONG_MAX).

--
greg

> Generally speaking looking at errno when you haven't received an error return
> from a libc function is asking for trouble. It could be leftover from any
> previous libc error.

Oh, sorry, just reread the code snippet and see that isn't an issue. nevermind.

--
greg

Greg Stark <gsstark(at)MIT(dot)EDU> writes:

> Generally speaking looking at errno when you haven't received an error return
> from a libc function is asking for trouble. It could be leftover from any
> previous libc error.
>
> That's how you get programs saying things like "strtol: No such file or
> directory" ...

Ah, I take back my taking back of this. It's still dicey to be doing it this
way -- even if you reset errno before calling the library function.

The problem is that the function can call other libc functions, which may in
turn return errors. But these errors might be incidental and handled by the
function you're calling.

The typical case of this is calling printf which first calls isatty(). printf
then returns success but leaves errno set to ENOTTY. And programs that check
errno without checking the return valule -- even if they clear it before
calling printf -- mysteriously print "Not a Typewriter" after correctly
printing the data.

The SUS standard protects against this though by only allowing this to happen
for functions that don't themselves use errno to signal errors:

The value of errno may be set to nonzero by a library function call
whether or not there is an error, provided the use of errno is not
documented in the description of the function in this International
Standard.

Older platforms may still have this behaviour, but strtol seems like a pretty
innocuous case. It's hard to imagine strtol needing to call much else. And
strtol was an ANSI addition so one imagines most platforms got it right from
the beginning.

--
greg

I started working on this problem on Tuesday, but was in New York City
yesterday so I was not able to comment on this patch before. I think
the one applied is great. (I added C comments on the use of "errno =
0".

Here is the patch I was working on. It does us a separate libpq
strtol() function, but I question whether it is worth it, or if it is
meaningful when used by FRONTEND applications. Anyway, I am just
throwing it out if it gives others ideas.

---------------------------------------------------------------------------

Michael Fuhr wrote:
> On Wed, Nov 30, 2005 at 02:01:46PM -0500, Tom Lane wrote:
> > Michael Fuhr <mike(at)fuhr(dot)org> writes:
> > > Any preferences on an approach? The simplest and easiest to verify
> > > would be to raise an error for just this particular case; a TODO
> > > item might be to change how the string is parsed to allow values
> > > larger than LONG_MAX.
> >
> > I think the latter would be a feature enhancement and therefore not
> > good material to back-patch. Just erroring out seems appropriate
> > for now.
>
> Agreed. I'm thinking about rewriting strtol() calls in datetime.c
> to look like this:
>
> errno = 0;
> val = strtol(field[i], &cp, 10);
> if (errno == ERANGE)
> return DTERR_FIELD_OVERFLOW;
>
> Does that look okay? Or would you rather raise an error with ereport()?
>
> > > I see several calls to strtol() that aren't checked for overflow but
> > > that might not be relevant to this problem, so I'm thinking this patch
> > > ought not touch them. Maybe that's another TODO item.
> >
> > If it's possible for them to be given overflowing input, they probably
> > ought to be checked.
>
> I'm looking at all the strtol() calls in datetime.c right now; I
> haven't looked anywhere else yet. Should I bother checking values
> that will be range checked later anyway? Time zone displacements,
> for example?
>
> --
> Michael Fuhr
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Greg Stark <gsstark(at)mit(dot)edu> writes:
> Ah, I take back my taking back of this. It's still dicey to be doing it this
> way -- even if you reset errno before calling the library function.

See the rest of the thread. The I-believe-what-I-read-in-the-spec camp
may take comfort from what it says in the SUS strtol spec,
http://www.opengroup.org/onlinepubs/007908799/xsh/strtol.html

The strtol() function will not change the setting of errno if
successful.

Those who are disinclined to assume that implementations always follow
the spec may take comfort from the fact that we've been testing errno
only (without also inspecting the return value) in pg_atoi for years
without problems.

From neither point of view do I see an argument for adding a test on the
return value. It's unnecessary according to the spec, and if you
disbelieve that strtol follows the spec, you should also be suspicious
about whether it's guaranteed to return LONG_MAX/LONG_MIN upon overflow
failure. We have no track record that gives evidence that the latter is
universally true.

regards, tom lane

On Thu, Dec 01, 2005 at 03:31:41PM -0500, Greg Stark wrote:
> Greg Stark <gsstark(at)MIT(dot)EDU> writes:
> > Generally speaking looking at errno when you haven't received an error return
> > from a libc function is asking for trouble. It could be leftover from any
> > previous libc error.
> >
> > That's how you get programs saying things like "strtol: No such file or
> > directory" ...
>
> Ah, I take back my taking back of this. It's still dicey to be doing it this
> way -- even if you reset errno before calling the library function.
>
> The problem is that the function can call other libc functions, which may in
> turn return errors. But these errors might be incidental and handled by the
> function you're calling.

I had that concern, as I've seen such incidental errno changes
before. But Tom pointed out the following from SUS:

The strtol() function shall not change the setting of errno if
successful.

Immediately after that the standard says:

Since 0, {LONG_MIN} or {LLONG_MIN}, and {LONG_MAX} or {LLONG_MAX}
are returned on error and are also valid returns on success, an
application wishing to check for error situations should set errno
to 0, then call strtol() or strtoll(), then check errno.

I don't know if any systems are non-compliant in this respect, but
Tom said that "we've been doing it that way (errno test only) for
many years without complaints."

--
Michael Fuhr

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Here is the patch I was working on. It does us a separate libpq
> strtol() function, but I question whether it is worth it, or if it is
> meaningful when used by FRONTEND applications. Anyway, I am just
> throwing it out if it gives others ideas.

I did look through all the other calls to strtol/strtoul, and concluded
that the ones in datetime.c were the only undefended ones that really
needed a check.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Here is the patch I was working on. It does us a separate libpq
> > strtol() function, but I question whether it is worth it, or if it is
> > meaningful when used by FRONTEND applications. Anyway, I am just
> > throwing it out if it gives others ideas.
>
> I did look through all the other calls to strtol/strtoul, and concluded
> that the ones in datetime.c were the only undefended ones that really
> needed a check.

Yea, I think I agree.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073