Thread: 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
Attachment
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@samurai.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@fuhr.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@commandprompt.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@fuhr.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/browse > r/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 ... Yes, and I for one love it :-) Now if I could just access the actual svn repository as well, I'd be even happier... //Magnus
> 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
Magnus Hagander wrote: >>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 ... >> >> > >Yes, and I for one love it :-) > >Now if I could just access the actual svn repository as well, I'd be >even happier... > > > > I have added it to the set of useful links on pgfoundry's front page. cheers andrew
>>> >>> 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 ... >>> >> I need to spend some time on it to see if there is a way that I can convert more then the whole tree at a time within the same repository. Right now we have to convert the whole thing which takes quite a bit of time. Oh and don't forget to use the RSS capabilities. >> >> Yes, and I for one love it :-) >> >> Now if I could just access the actual svn repository as well, I'd be >> even happier... > We are also happy to provide accounts to those who would like to actually use the wiki to discuss specific aspects of the code. This would be more for wannabe hacker edibility, tips, tricks etc... >> > > I have added it to the set of useful links on pgfoundry's front page. Cool! Joshua D. Drake > > cheers > > andrew > > ---------------------------(end of broadcast)--------------------------- > TIP 6: explain analyze is your friend -- The PostgreSQL Company - Command Prompt, Inc. 1.503.667.4564 PostgreSQL Replication, Consulting, Custom Development, 24x7 support Managed Services, Shared and Dedicated Hosting Co-Authors: PLphp, PLperl - http://www.commandprompt.com/