Re: [HACKERS] money type overflow checks

[Peter Eisentraut]

On 8/5/16 1:14 PM, Tom Lane wrote:
> No, I don't think it's sufficient after a multiplication by 10.  That
> would be enough to shift some bits clear out of the word, but there's
> no certainty that the new sign bit would be 1.
> 
> The scheme used in scanint8 is safe.  But I think it was written that way
> mainly to avoid hard-wired assumptions about how wide int64 is, a
> consideration that's a mite obsolete now.

OK, I did it like int8, and added more tests.  My original patch didn't
get the most negative integer right.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 7c902e5f9a9051923df7d343cb00c93e6774a16e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 11 Aug 2016 12:00:00 -0400
Subject: [PATCH v2] Add overflow checks to money type input function

The money type input function did not have any overflow checks at all.
There were some regression tests that purported to check for overflow,
but they actually checked for the overflow behavior of the int8 type
before casting to money.  Remove those unnecessary checks and add some
that actually check the money input function.
---
 src/backend/utils/adt/cash.c| 51 --
 src/test/regress/expected/money.out | 85 +
 src/test/regress/sql/money.sql  | 26 ++--
 3 files changed, 146 insertions(+), 16 deletions(-)

diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index b336185..b2676e8 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -189,13 +189,30 @@ cash_in(PG_FUNCTION_ARGS)
 	printf("cashin- string is '%s'\n", s);
 #endif
 
+	/*
+	 * We accumulate the absolute amount in "value" and then apply the sign at
+	 * the end.  (The sign can appear before or after the digits, so it would
+	 * be more complicated to do otherwise.)  Because of the larger range of
+	 * negative signed integers, we build "value" in the negative and then
+	 * flip the sign at the end, catching most-negative-number overflow if
+	 * necessary.
+	 */
+
 	for (; *s; s++)
 	{
 		/* we look for digits as long as we have found less */
 		/* than the required number of decimal places */
 		if (isdigit((unsigned char) *s) && (!seen_dot || dec < fpoint))
 		{
-			value = (value * 10) + (*s - '0');
+			Cash newvalue = (value * 10) - (*s - '0');
+
+			if (newvalue / 10 != value)
+ereport(ERROR,
+		(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+		 errmsg("value \"%s\" is out of range for type money",
+str)));
+
+			value = newvalue;
 
 			if (seen_dot)
 dec++;
@@ -214,11 +231,27 @@ cash_in(PG_FUNCTION_ARGS)
 
 	/* round off if there's another digit */
 	if (isdigit((unsigned char) *s) && *s >= '5')
-		value++;
+		value--;
+
+	if (value > 0)
+		ereport(ERROR,
+(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("value \"%s\" is out of range for type money",
+		str)));
 
 	/* adjust for less than required decimal places */
 	for (; dec < fpoint; dec++)
-		value *= 10;
+	{
+		Cash newvalue = value * 10;
+
+		if (newvalue / 10 != value)
+			ereport(ERROR,
+	(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+	 errmsg("value \"%s\" is out of range for type money",
+			str)));
+
+		value = newvalue;
+	}
 
 	/*
 	 * should only be trailing digits followed by whitespace, right paren,
@@ -247,7 +280,17 @@ cash_in(PG_FUNCTION_ARGS)
 			str)));
 	}
 
-	result = value * sgn;
+	if (sgn > 0)
+	{
+		result = -value;
+		if (result < 0)
+			ereport(ERROR,
+	(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+	 errmsg("value \"%s\" is out of range for type money",
+			str)));
+	}
+	else
+		result = value;
 
 #ifdef CASHDEBUG
 	printf("cashin- result is " INT64_FORMAT "\n", result);
diff --git a/src/test/regress/expected/money.out b/src/test/regress/expected/money.out
index 538235c..3be209d 100644
--- a/src/test/regress/expected/money.out
+++ b/src/test/regress/expected/money.out
@@ -185,6 +185,83 @@ SELECT * FROM money_data;
  $123.46
 (1 row)
 
+-- input checks
+SELECT '1234567890'::money;
+   money   
+---
+ $1,234,567,890.00
+(1 row)
+
+SELECT '12345678901234567'::money;
+   money
+
+ $12,345,678,901,234,567.00
+(1 row)
+
+SELECT '123456789012345678'::money;
+ERROR:  value "123456789012345678" is out of range for type money
+LINE 1: SELECT '123456789012345678'::money;
+   ^
+SELECT '9223372036854775807'::money;
+ERROR:  value "9223372036854775807" is out of range for type money
+LINE 1: SELECT '9223372036854775807'::money;
+   ^
+SELECT '-12345'::money;
+money
+-
+ -$12,345.00
+(1 row)
+
+SELECT '-1234567890'::money;
+   money
+
+ -$1,234,567,890.00
+(1 row)
+
+SELECT '-12345678901234567'::money;
+money
+-
+ 

Re: [HACKERS] money type overflow checks

[Tom Lane]

Peter Eisentraut  writes:
> The input function of the money type has no overflow checks:

Ugh.

> (Is checking for < 0 a valid overflow check?

No, I don't think it's sufficient after a multiplication by 10.  That
would be enough to shift some bits clear out of the word, but there's
no certainty that the new sign bit would be 1.

The scheme used in scanint8 is safe.  But I think it was written that way
mainly to avoid hard-wired assumptions about how wide int64 is, a
consideration that's a mite obsolete now.  You could possibly avoid the
cost of a division by relying on comparisons to PG_INT64_MAX/10.

regards, tom lane


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

[HACKERS] money type overflow checks

[Peter Eisentraut]

The input function of the money type has no overflow checks:

=> select '12345678901234567890'::money;
money
-
 -$13,639,628,150,831,692.72
(1 row)

The tests in the regression test file money.sql are bogus because they
only test the overflow checks of the bigint type before the cast.

Here is a patch that adds appropriate checks and tests.  We could
probably remove the bogus tests.

(Is checking for < 0 a valid overflow check?  We save the sign until the
very end, so it ought to work.  The code in int8.c works differently there.)

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 6fcce1d6c3685cfb5bacdb89981d4f6a3911dded Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 5 Aug 2016 11:50:53 -0400
Subject: [PATCH] Add overflow checks to money type input function

---
 src/backend/utils/adt/cash.c| 18 ++
 src/test/regress/expected/money.out | 47 +
 src/test/regress/sql/money.sql  | 11 +
 3 files changed, 76 insertions(+)

diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index b336185..0c06f71 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -197,6 +197,12 @@ cash_in(PG_FUNCTION_ARGS)
 		{
 			value = (value * 10) + (*s - '0');
 
+			if (value < 0)
+ereport(ERROR,
+		(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+		 errmsg("value \"%s\" is out of range for type money",
+str)));
+
 			if (seen_dot)
 dec++;
 		}
@@ -216,10 +222,22 @@ cash_in(PG_FUNCTION_ARGS)
 	if (isdigit((unsigned char) *s) && *s >= '5')
 		value++;
 
+	if (value < 0)
+		ereport(ERROR,
+(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("value \"%s\" is out of range for type money",
+		str)));
+
 	/* adjust for less than required decimal places */
 	for (; dec < fpoint; dec++)
 		value *= 10;
 
+	if (value < 0)
+		ereport(ERROR,
+(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("value \"%s\" is out of range for type money",
+		str)));
+
 	/*
 	 * should only be trailing digits followed by whitespace, right paren,
 	 * trailing sign, and/or trailing currency symbol
diff --git a/src/test/regress/expected/money.out b/src/test/regress/expected/money.out
index 538235c..206f5c4 100644
--- a/src/test/regress/expected/money.out
+++ b/src/test/regress/expected/money.out
@@ -185,6 +185,53 @@ SELECT * FROM money_data;
  $123.46
 (1 row)
 
+-- input checks
+SELECT '1234567890'::money;
+   money   
+---
+ $1,234,567,890.00
+(1 row)
+
+SELECT '12345678901234567'::money;
+   money
+
+ $12,345,678,901,234,567.00
+(1 row)
+
+SELECT '123456789012345678'::money;
+ERROR:  value "123456789012345678" is out of range for type money
+LINE 1: SELECT '123456789012345678'::money;
+   ^
+SELECT '9223372036854775807'::money;
+ERROR:  value "9223372036854775807" is out of range for type money
+LINE 1: SELECT '9223372036854775807'::money;
+   ^
+SELECT '-12345'::money;
+money
+-
+ -$12,345.00
+(1 row)
+
+SELECT '-1234567890'::money;
+   money
+
+ -$1,234,567,890.00
+(1 row)
+
+SELECT '-12345678901234567'::money;
+money
+-
+ -$12,345,678,901,234,567.00
+(1 row)
+
+SELECT '-123456789012345678'::money;
+ERROR:  value "-123456789012345678" is out of range for type money
+LINE 1: SELECT '-123456789012345678'::money;
+   ^
+SELECT '-9223372036854775808'::money;
+ERROR:  value "-9223372036854775808" is out of range for type money
+LINE 1: SELECT '-9223372036854775808'::money;
+   ^
 -- Cast int4/int8 to money
 SELECT 1234567890::money;
money   
diff --git a/src/test/regress/sql/money.sql b/src/test/regress/sql/money.sql
index 09b9476..d07a616 100644
--- a/src/test/regress/sql/money.sql
+++ b/src/test/regress/sql/money.sql
@@ -57,6 +57,17 @@ CREATE TABLE money_data (m money);
 INSERT INTO money_data VALUES ('$123.459');
 SELECT * FROM money_data;
 
+-- input checks
+SELECT '1234567890'::money;
+SELECT '12345678901234567'::money;
+SELECT '123456789012345678'::money;
+SELECT '9223372036854775807'::money;
+SELECT '-12345'::money;
+SELECT '-1234567890'::money;
+SELECT '-12345678901234567'::money;
+SELECT '-123456789012345678'::money;
+SELECT '-9223372036854775808'::money;
+
 -- Cast int4/int8 to money
 SELECT 1234567890::money;
 SELECT 12345678901234567::money;
-- 
2.9.2


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