Re: Check for integer overflow in datetime functions

Check integer conversion for overflow in datetime functions. Problem
reported by Christopher Kings-Lynne:

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

In one place (line 60 in the patch) the code sets errno to 0 when
it should still be 0 after similar code a few lines above (otherwise
the function would have returned an error). I wasn't sure what
would be preferred: clearing errno again "just to be sure," a comment
explaining why it isn't necessary, or nothing at all.

This patch should apply cleanly against HEAD and 8.1. If the patch
looks good then I'll submit patches for earlier branches when I get
a chance to build and test those versions.

--
Michael Fuhr

On Wed, 2005-11-30 at 19:36 -0700, Michael Fuhr wrote:
> Check integer conversion for overflow in datetime functions.

It seems a bit laborious to always manually set errno to zero before
invoking strtol() (both in the places added by the patch, and all the
places that already did that). While it's only a minor notational
improvement, I wonder if it would be worth adding a pg_strtol() that did
the errno assignment so that each call-site wouldn't need to worry about
it?

-Neil

Neil Conway <neilc(at)samurai(dot)com> writes:
> It seems a bit laborious to always manually set errno to zero before
> invoking strtol() (both in the places added by the patch, and all the
> places that already did that). While it's only a minor notational
> improvement, I wonder if it would be worth adding a pg_strtol() that did
> the errno assignment so that each call-site wouldn't need to worry about
> it?

Don't see the point really, since the check of errno would still have to
be in the calling code (since pg_strtol wouldn't know what the
appropriate error action is). So the choice is between

errno = 0;
foo = strtol(...);
if (errno == ERANGE)
... something ...

and

foo = pg_strtol(...);
if (errno == ERANGE)
... something ...

I don't think there's less cognitive load in the second case,
particularly not to someone who's familiar with the standard behavior
of strtol() --- you'll be constantly thinking "what if errno is already
ERANGE ... oh, pg_strtol resets it ..."

I'd be in favor of encapsulating the whole sequence including an
ereport(ERROR) into one function, except it seems we'd not use it in
very many places.

BTW, Neil, were you looking at this with intention to commit it?
I'll pick it up if not, but don't want to duplicate effort if you've
already started on it.

regards, tom lane

Michael Fuhr <mike(at)fuhr(dot)org> writes:
> Check integer conversion for overflow in datetime functions.

Looks reasonable, will check and apply.

> This patch should apply cleanly against HEAD and 8.1. If the patch
> looks good then I'll submit patches for earlier branches when I get
> a chance to build and test those versions.

Don't worry about that, I'll take care of it. I prefer committing all
the branches at once when doing a multi-branch fix (less clutter in
the CVS logs).

regards, tom lane

Tom Lane wrote:

> Don't worry about that, I'll take care of it. I prefer committing all
> the branches at once when doing a multi-branch fix (less clutter in
> the CVS logs).

How do you do that? I have multiple checked-out trees, I assume you do
the same and just handle the simultaneous-ness by hand?

BTW, has anyone checked Command Prompt's Subversion repository? It's a
mirror of our anonymous CVS (AFAICT). I'm using it for reading diffs
lately, and it's much nicer to look at the whole patch as a single diff
rather than going a single file at a time.

http://projects.commandprompt.com/projects/public/pgsql/browser/trunk/pgsql

It has the additional advantage over our current CVSweb that it's set
with tabs to 4 spaces, so it looks just like our code is supposed to ...

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

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane wrote:
>> Don't worry about that, I'll take care of it. I prefer committing all
>> the branches at once when doing a multi-branch fix (less clutter in
>> the CVS logs).

> How do you do that? I have multiple checked-out trees, I assume you do
> the same and just handle the simultaneous-ness by hand?

Well, they're not *exactly* simultaneous of course. I set up the
modified files in each tree and then commit, commit, commit (the -F
option helps if the commit message is long). I use cvs2cl to extract
CVS history, and it will fold together commits in different branches
if they have the same commit message and are within some time delta
of each other ... I think it's 5 minutes or so.

regards, tom lane

Michael Fuhr <mike(at)fuhr(dot)org> writes:
> Check integer conversion for overflow in datetime functions.

Applied back to 7.4. The patch would not work in 7.3 because we didn't
have the DTERR_ convention back then, and it seems not worth the effort
to try to deal with the problem that far back.

> In one place (line 60 in the patch) the code sets errno to 0 when
> it should still be 0 after similar code a few lines above (otherwise
> the function would have returned an error). I wasn't sure what
> would be preferred: clearing errno again "just to be sure," a comment
> explaining why it isn't necessary, or nothing at all.

I left it as-is ...

regards, tom lane

> BTW, has anyone checked Command Prompt's Subversion repository? It's a
> mirror of our anonymous CVS (AFAICT). I'm using it for reading diffs
> lately, and it's much nicer to look at the whole patch as a single diff
> rather than going a single file at a time.
>
> http://projects.commandprompt.com/projects/public/pgsql/browser/trunk/pgsql
>
> It has the additional advantage over our current CVSweb that it's set
> with tabs to 4 spaces, so it looks just like our code is supposed to ...

Does anyone else find the PostgreSQL "TRUNK" funny? :D