On 17 November 2015 at 14:43, Tom Lane <[email protected]> wrote:
> Dean Rasheed <[email protected]> writes:
>> I just noticed that div_var_fast() has almost identical code, and so
>> in principle it has the same vulnerability, although it obviously only
>> affects the transcendental functions.
>> I don't actually have a test case that triggers it, but it's basically
>> the same algorithm, so logically it needs the same additional headroom
>> to avoid a possible overflow.
>
> Hm, good point. I don't feel a compulsion to have a test case that
> proves it's broken before we fix it. Do you want to send a patch?
>
OK, here it is. It's slightly different from mul_var, because maxdiv
is tracking absolute values and the carry may be positive or negative,
and it's absolute value may be as high as INT_MAX / NBASE + 1 (when
it's negative), but otherwise the principle is the same.
Regards,
Dean
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
new file mode 100644
index fe9f3f7..eaafcd9
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -6266,8 +6266,15 @@ div_var_fast(NumericVar *var1, NumericVa
/*
* maxdiv tracks the maximum possible absolute value of any div[] entry;
* when this threatens to exceed INT_MAX, we take the time to propagate
- * carries. To avoid overflow in maxdiv itself, it actually represents
- * the max possible abs. value divided by NBASE-1.
+ * carries. Furthermore, we need to ensure that overflow doesn't occur
+ * during the carry propagation passes either. The carry values may have
+ * an absolute value as high as INT_MAX/NBASE + 1, so really we must
+ * normalize when digits threaten to exceed INT_MAX - INT_MAX/NBASE - 1.
+ *
+ * To avoid overflow in maxdiv itself, it actually represents the max
+ * possible absolute value divided by NBASE-1, ie, at the top of the loop
+ * it is known that no dig[] entry has an absolute value exceeding
+ * maxdiv * (NBASE-1).
*/
maxdiv = 1;
@@ -6293,7 +6300,7 @@ div_var_fast(NumericVar *var1, NumericVa
{
/* Do we need to normalize now? */
maxdiv += Abs(qdigit);
- if (maxdiv > INT_MAX / (NBASE - 1))
+ if (maxdiv > (INT_MAX - INT_MAX / NBASE - 1) / (NBASE - 1))
{
/* Yes, do it */
carry = 0;
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
