pgsql: Fix integer-overflow problems in interval comparison.

Started by Tom Laneabout 9 years ago4 messagescomitters
Jump to latest
#1Tom Lane
tgl@sss.pgh.pa.us

Fix integer-overflow problems in interval comparison.

When using integer timestamps, the interval-comparison functions tried
to compute the overall magnitude of an interval as an int64 number of
microseconds. As reported by Frazer McLean, this overflows for intervals
exceeding about 296000 years, which is bad since we nominally allow
intervals many times larger than that. That results in wrong comparison
results, and possibly in corrupted btree indexes for columns containing
such large interval values.

To fix, compute the magnitude as int128 instead. Although some compilers
have native support for int128 calculations, many don't, so create our
own support functions that can do 128-bit addition and multiplication
if the compiler support isn't there. These support functions are designed
with an eye to allowing the int128 code paths in numeric.c to be rewritten
for use on all platforms, although this patch doesn't do that, or even
provide all the int128 primitives that will be needed for it.

Back-patch as far as 9.4. Earlier releases did not guard against overflow
of interval values at all (commit 146604ec4 fixed that), so it seems not
very exciting to worry about overly-large intervals for them.

Before 9.6, we did not assume that unreferenced "static inline" functions
would not draw compiler warnings, so omit functions not directly referenced
by timestamp.c, the only present consumer of int128.h. (We could have
omitted these functions in HEAD too, but since they were written and
debugged on the way to the present patch, and they look likely to be needed
by numeric.c, let's keep them in HEAD.) I did not bother to try to prevent
such warnings in a --disable-integer-datetimes build, though.

Before 9.5, configure will never define HAVE_INT128, so the part of
int128.h that exploits a native int128 implementation is dead code in the
9.4 branch. I didn't bother to remove it, thinking that keeping the file
looking similar in different branches is more useful.

In HEAD only, add a simple test harness for int128.h in src/tools/.

In back branches, this does not change the float-timestamps code path.
That's not subject to the same kind of overflow risk, since it computes
the interval magnitude as float8. (No doubt, when this code was originally
written, overflow was disregarded for exactly that reason.) There is a
precision hazard instead :-(, but we'll avert our eyes from that question,
since no complaints have been reported and that code's deprecated anyway.

Kyotaro Horiguchi and Tom Lane

Discussion: /messages/by-id/1490104629.422698.918452336.26FA96B7@webmail.messagingengine.com

Branch
------
REL9_4_STABLE

Details
-------
http://git.postgresql.org/pg/commitdiff/8851bcf8813baa0ea393ef9d2894d15b3f13f957

Modified Files
--------------
src/backend/utils/adt/timestamp.c | 65 ++++++++--
src/include/common/int128.h | 231 +++++++++++++++++++++++++++++++++
src/test/regress/expected/interval.out | 64 +++++++++
src/test/regress/sql/interval.sql | 27 ++++
4 files changed, 376 insertions(+), 11 deletions(-)

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#2Peter Eisentraut
peter_e@gmx.net
In reply to: Tom Lane (#1)
Re: pgsql: Fix integer-overflow problems in interval comparison.

On 4/5/17 23:51, Tom Lane wrote:

Fix integer-overflow problems in interval comparison.

Branch
------
REL9_4_STABLE

Details
-------
http://git.postgresql.org/pg/commitdiff/8851bcf8813baa0ea393ef9d2894d15b3f13f957

Modified Files
--------------
src/backend/utils/adt/timestamp.c | 65 ++++++++--
src/include/common/int128.h | 231 +++++++++++++++++++++++++++++++++
src/test/regress/expected/interval.out | 64 +++++++++
src/test/regress/sql/interval.sql | 27 ++++
4 files changed, 376 insertions(+), 11 deletions(-)

This is failing cpluspluscheck now because C++ does not have
_Static_assert, which is used in int128.h.

I suppose this header is not intended for public consumption, at least
in the back branches, so it would be OK to exclude it from the check.
Perhaps a different solution would be appropriate in master.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#2)
Re: pgsql: Fix integer-overflow problems in interval comparison.

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 4/5/17 23:51, Tom Lane wrote:

Fix integer-overflow problems in interval comparison.

This is failing cpluspluscheck now because C++ does not have
_Static_assert, which is used in int128.h.

Hmm. We could drop that assert, or move it to some .c file, or
wrap it in "#ifndef __cplusplus" ... but really it seems like a
more generic solution would be appropriate. This won't be the
last time somebody tries to do that, what with our increasing
use of inline functions.

Maybe the definition of StaticAssertStmt should be tweaked based on
__cplusplus? Or maybe the problem is cpluspluscheck's test methodology,
ie you ought to run configure with CC=c++ before trying to compile the
headers?

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#4Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#3)
Re: pgsql: Fix integer-overflow problems in interval comparison.

On 2017-04-17 13:04:31 -0400, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 4/5/17 23:51, Tom Lane wrote:

Fix integer-overflow problems in interval comparison.

This is failing cpluspluscheck now because C++ does not have
_Static_assert, which is used in int128.h.

Hmm. We could drop that assert, or move it to some .c file, or
wrap it in "#ifndef __cplusplus" ... but really it seems like a
more generic solution would be appropriate. This won't be the
last time somebody tries to do that, what with our increasing
use of inline functions.

Maybe the definition of StaticAssertStmt should be tweaked based on
__cplusplus?

That seems reasonable, possibly with a cplusplus version check and then
using static_assert()? Although I'd personally just mention that as a
todo for later.

Or maybe the problem is cpluspluscheck's test methodology,
ie you ought to run configure with CC=c++ before trying to compile the
headers?

That seems unlikely to work well for the moment...

- Andres

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers