Re: [PATCH v2] Add overflow checks to money type input function
Hello Peter,
My 0.02ￜ (not $0.02:-) comments on this patch:
Patch applies and "make check" is ok. I see no issue with the code.
A few comments below.
The regression tests are clearer & commented, it is an improvement.
While you are at it, maybe you could consider adding tests for more
features, eg ',' skipping and '(' as negative sign:
SELECT '(1)'::MONEY;
SELECT '($123,456.78)'::MONEY;
The code does what it advertises. Note that this patch only tests
overflows when parsing a string. It does not detect overflows during
operations.
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.
I think that the lack of generality of the MONEY type makes it near
unusable (I do not think that it is the place of the database to
prettyprint the currency, especially with a '$' sign which happen not to
be the currency of 95% of the world population, the precision is hardwired
to 2 figures after the unit, the convention to use '(' for negative
numbers is rather an anglo-saxon accounting one, ...), so I would not have
bothered. This type should really be named "DOLLAR" or "USD".
+ /* + * 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,
Argh. A trick!
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)
I would have done "if (newvalue > 0)" because / used to be expensive and
the overflow materializes as a sign inversion, but I understand Tom
commented against that, so this is fine.
/* round off if there's another digit */ if (isdigit((unsigned char) *s) && *s >= '5') - value++; + value--;
Positive/negative trick again. A reminder comment?
+ if (value > 0)
Trick again...
Ok, this test seems to be necessary just for a min int value that would
have been badly rounded down by the preceding increment.
+ 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;
The code looks a little bit strange because of the above negative value
trick. Maybe there could be a reminder comment?
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I have updated the patch with additional tests and comments per your
review. Final(?) version attached.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v3-0001-Add-overflow-checks-to-money-type-input-function.patchtext/x-patch; name=v3-0001-Add-overflow-checks-to-money-type-input-function.patchDownload
From ee34d7d64a4b10c9f7fbe8c905a56cea1584c8c9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Thu, 8 Sep 2016 12:00:00 -0400
Subject: [PATCH v3] 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.
Reviewed-by: Fabien COELHO <coelho@cri.ensmp.fr>
---
src/backend/utils/adt/cash.c | 53 ++++++++++++++++++--
src/test/regress/expected/money.out | 98 ++++++++++++++++++++++++++++++++++---
src/test/regress/sql/money.sql | 30 ++++++++++--
3 files changed, 165 insertions(+), 16 deletions(-)
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index b336185..a146b0a 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--; /* remember we build the value in the negative */
+
+ 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,19 @@ cash_in(PG_FUNCTION_ARGS)
str)));
}
- result = value * sgn;
+ /* If the value is supposed to be positive, flip the sign, but check for
+ * the most negative number. */
+ 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..5695f87 100644
--- a/src/test/regress/expected/money.out
+++ b/src/test/regress/expected/money.out
@@ -185,6 +185,96 @@ 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;
+ ^
+-- special characters
+SELECT '(1)'::money;
+ money
+--------
+ -$1.00
+(1 row)
+
+SELECT '($123,456.78)'::money;
+ money
+--------------
+ -$123,456.78
+(1 row)
+
+-- documented minimums and maximums
+SELECT '-92233720368547758.08'::money;
+ money
+-----------------------------
+ -$92,233,720,368,547,758.08
+(1 row)
+
+SELECT '92233720368547758.07'::money;
+ money
+----------------------------
+ $92,233,720,368,547,758.07
+(1 row)
+
+SELECT '-92233720368547758.09'::money;
+ERROR: value "-92233720368547758.09" is out of range for type money
+LINE 1: SELECT '-92233720368547758.09'::money;
+ ^
+SELECT '92233720368547758.08'::money;
+ERROR: value "92233720368547758.08" is out of range for type money
+LINE 1: SELECT '92233720368547758.08'::money;
+ ^
+-- rounding
+SELECT '-92233720368547758.085'::money;
+ERROR: value "-92233720368547758.085" is out of range for type money
+LINE 1: SELECT '-92233720368547758.085'::money;
+ ^
+SELECT '92233720368547758.075'::money;
+ERROR: value "92233720368547758.075" is out of range for type money
+LINE 1: SELECT '92233720368547758.075'::money;
+ ^
-- Cast int4/int8 to money
SELECT 1234567890::money;
money
@@ -198,10 +288,6 @@ SELECT 12345678901234567::money;
$12,345,678,901,234,567.00
(1 row)
-SELECT 123456789012345678::money;
-ERROR: bigint out of range
-SELECT 9223372036854775807::money;
-ERROR: bigint out of range
SELECT (-12345)::money;
money
-------------
@@ -220,10 +306,6 @@ SELECT (-12345678901234567)::money;
-$12,345,678,901,234,567.00
(1 row)
-SELECT (-123456789012345678)::money;
-ERROR: bigint out of range
-SELECT (-9223372036854775808)::money;
-ERROR: bigint out of range
SELECT 1234567890::int4::money;
money
-------------------
diff --git a/src/test/regress/sql/money.sql b/src/test/regress/sql/money.sql
index 09b9476..561ccb5 100644
--- a/src/test/regress/sql/money.sql
+++ b/src/test/regress/sql/money.sql
@@ -57,16 +57,38 @@ 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;
+
+-- special characters
+SELECT '(1)'::money;
+SELECT '($123,456.78)'::money;
+
+-- documented minimums and maximums
+SELECT '-92233720368547758.08'::money;
+SELECT '92233720368547758.07'::money;
+
+SELECT '-92233720368547758.09'::money;
+SELECT '92233720368547758.08'::money;
+
+-- rounding
+SELECT '-92233720368547758.085'::money;
+SELECT '92233720368547758.075'::money;
+
-- Cast int4/int8 to money
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;
SELECT 1234567890::int4::money;
SELECT 12345678901234567::int8::money;
SELECT (-1234567890)::int4::money;
--
2.10.0
I have updated the patch with additional tests and comments per your
review. Final(?) version attached.
Applied on head, make check ok. No more comments on the code which does
what it says. I'm fine with this patch.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 9/9/16 3:19 AM, Fabien COELHO wrote:
I have updated the patch with additional tests and comments per your
review. Final(?) version attached.Applied on head, make check ok. No more comments on the code which does
what it says. I'm fine with this patch.
Pushed, thanks.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers