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@fuhr.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@fuhr.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@fuhr.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@fuhr.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@fuhr.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@fuhr.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@fuhr.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@fuhr.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@fuhr.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.htmlThe 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@fuhr.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.htmlThe 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@sss.pgh.pa.us> writes:
Michael Fuhr <mike@fuhr.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@MIT.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@fuhr.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@postgresql.org so that your
message can get through to the mailing list cleanly
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Attachments:
/rtmp/difftext/plainDownload
Index: src/backend/libpq/ip.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/libpq/ip.c,v
retrieving revision 1.33
diff -c -c -r1.33 ip.c
*** src/backend/libpq/ip.c 22 Nov 2005 18:17:11 -0000 1.33
--- src/backend/libpq/ip.c 1 Dec 2005 18:40:02 -0000
***************
*** 342,348 ****
long bits;
char *endptr;
! bits = strtol(numbits, &endptr, 10);
if (*numbits == '\0' || *endptr != '\0')
return -1;
--- 342,348 ----
long bits;
char *endptr;
! bits = pg_strtol_range(numbits, &endptr, 10);
if (*numbits == '\0' || *endptr != '\0')
return -1;
Index: src/backend/utils/adt/datetime.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/datetime.c,v
retrieving revision 1.161
diff -c -c -r1.161 datetime.c
*** src/backend/utils/adt/datetime.c 22 Nov 2005 18:17:22 -0000 1.161
--- src/backend/utils/adt/datetime.c 1 Dec 2005 18:40:03 -0000
***************
*** 1013,1019 ****
if (tzp == NULL)
return DTERR_BAD_FORMAT;
! val = strtol(field[i], &cp, 10);
j2date(val, &tm->tm_year, &tm->tm_mon, &tm->tm_mday);
/* Get the time zone from the end of the string */
--- 1013,1019 ----
if (tzp == NULL)
return DTERR_BAD_FORMAT;
! val = pg_strtol_range(field[i], &cp, 10);
j2date(val, &tm->tm_year, &tm->tm_mon, &tm->tm_mday);
/* Get the time zone from the end of the string */
***************
*** 1158,1164 ****
char *cp;
int val;
! val = strtol(field[i], &cp, 10);
/*
* only a few kinds are allowed to have an embedded
--- 1158,1164 ----
char *cp;
int val;
! val = pg_strtol_range(field[i], &cp, 10);
/*
* only a few kinds are allowed to have an embedded
***************
*** 1915,1921 ****
break;
}
! val = strtol(field[i], &cp, 10);
/*
* only a few kinds are allowed to have an embedded
--- 1915,1921 ----
break;
}
! val = pg_strtol_range(field[i], &cp, 10);
/*
* only a few kinds are allowed to have an embedded
***************
*** 2456,2466 ****
*tmask = DTK_TIME_M;
! tm->tm_hour = strtol(str, &cp, 10);
if (*cp != ':')
return DTERR_BAD_FORMAT;
str = cp + 1;
! tm->tm_min = strtol(str, &cp, 10);
if (*cp == '\0')
{
tm->tm_sec = 0;
--- 2456,2466 ----
*tmask = DTK_TIME_M;
! tm->tm_hour = pg_strtol_range(str, &cp, 10);
if (*cp != ':')
return DTERR_BAD_FORMAT;
str = cp + 1;
! tm->tm_min = pg_strtol_range(str, &cp, 10);
if (*cp == '\0')
{
tm->tm_sec = 0;
***************
*** 2471,2477 ****
else
{
str = cp + 1;
! tm->tm_sec = strtol(str, &cp, 10);
if (*cp == '\0')
*fsec = 0;
else if (*cp == '.')
--- 2471,2477 ----
else
{
str = cp + 1;
! tm->tm_sec = pg_strtol_range(str, &cp, 10);
if (*cp == '\0')
*fsec = 0;
else if (*cp == '.')
***************
*** 2522,2528 ****
*tmask = 0;
! val = strtol(str, &cp, 10);
if (cp == str)
return DTERR_BAD_FORMAT;
--- 2522,2528 ----
*tmask = 0;
! val = pg_strtol_range(str, &cp, 10);
if (cp == str)
return DTERR_BAD_FORMAT;
***************
*** 2809,2819 ****
if (*str != '+' && *str != '-')
return DTERR_BAD_FORMAT;
! hr = strtol(str + 1, &cp, 10);
/* explicit delimiter? */
if (*cp == ':')
! min = strtol(cp + 1, &cp, 10);
/* otherwise, might have run things together... */
else if (*cp == '\0' && strlen(str) > 3)
{
--- 2809,2819 ----
if (*str != '+' && *str != '-')
return DTERR_BAD_FORMAT;
! hr = pg_strtol_range(str + 1, &cp, 10);
/* explicit delimiter? */
if (*cp == ':')
! min = pg_strtol_range(cp + 1, &cp, 10);
/* otherwise, might have run things together... */
else if (*cp == '\0' && strlen(str) > 3)
{
***************
*** 3056,3062 ****
case DTK_DATE:
case DTK_NUMBER:
! val = strtol(field[i], &cp, 10);
if (type == IGNORE_DTF)
type = DTK_SECOND;
--- 3056,3062 ----
case DTK_DATE:
case DTK_NUMBER:
! val = pg_strtol_range(field[i], &cp, 10);
if (type == IGNORE_DTF)
type = DTK_SECOND;
Index: src/backend/utils/adt/numeric.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/numeric.c,v
retrieving revision 1.88
diff -c -c -r1.88 numeric.c
*** src/backend/utils/adt/numeric.c 22 Nov 2005 18:17:23 -0000 1.88
--- src/backend/utils/adt/numeric.c 1 Dec 2005 18:40:08 -0000
***************
*** 2812,2818 ****
char *endptr;
cp++;
! exponent = strtol(cp, &endptr, 10);
if (endptr == cp)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
--- 2812,2818 ----
char *endptr;
cp++;
! exponent = pg_strtol_range(cp, &endptr, 10);
if (endptr == cp)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
Index: src/include/port.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/port.h,v
retrieving revision 1.84
diff -c -c -r1.84 port.h
*** src/include/port.h 15 Oct 2005 02:49:41 -0000 1.84
--- src/include/port.h 1 Dec 2005 18:40:09 -0000
***************
*** 249,254 ****
--- 249,257 ----
#define closesocket close
#endif /* WIN32 */
+ /* do strtol(), but error out on ERANGE */
+ long pg_strtol_range(const char *nptr, char **endptr, int base);
+
/*
* Default "extern" declarations or macro substitutes for library routines.
* When necessary, these routines are provided by files in src/port/.
Index: src/interfaces/ecpg/pgtypeslib/numeric.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/ecpg/pgtypeslib/numeric.c,v
retrieving revision 1.24
diff -c -c -r1.24 numeric.c
*** src/interfaces/ecpg/pgtypeslib/numeric.c 22 Nov 2005 18:17:32 -0000 1.24
--- src/interfaces/ecpg/pgtypeslib/numeric.c 1 Dec 2005 18:40:11 -0000
***************
*** 218,224 ****
char *endptr;
(*ptr)++;
! exponent = strtol(*ptr, &endptr, 10);
if (endptr == (*ptr))
{
errno = PGTYPES_NUM_BAD_NUMERIC;
--- 218,224 ----
char *endptr;
(*ptr)++;
! exponent = pg_strtol_range(*ptr, &endptr, 10);
if (endptr == (*ptr))
{
errno = PGTYPES_NUM_BAD_NUMERIC;
Index: src/port/exec.c
===================================================================
RCS file: /cvsroot/pgsql/src/port/exec.c,v
retrieving revision 1.40
diff -c -c -r1.40 exec.c
*** src/port/exec.c 22 Nov 2005 18:17:34 -0000 1.40
--- src/port/exec.c 1 Dec 2005 18:40:11 -0000
***************
*** 592,594 ****
--- 592,618 ----
return -1;
}
+
+
+ /*
+ * This really should be in its own file, but it so small, it hardly
+ * seems worth it.
+ */
+ long
+ pg_strtol_range(const char *nptr, char **endptr, int base)
+ {
+ long ret = strtol(nptr, endptr, base);
+
+ if (errno == ERANGE)
+ {
+ #ifndef FRONTEND
+ elog(ERROR, _("value not in supported range for an integer: \"%s\""), nptr);
+ #else
+ /* Keep strings identical for ease of translation */
+ fprintf(stderr, _("value not in supported range for an integer: \"%s\""), nptr);
+ fputc('\n', stderr);
+ #endif
+ return LONG_MAX;
+ }
+ return ret;
+ }
Greg Stark <gsstark@mit.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@MIT.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@candle.pha.pa.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@candle.pha.pa.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@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073