fix: overflow in malformed interval

[dvarrazzo]

Close #1835

@michaelknap does it seem ok?

[michaelknap]

Thanks for the quick fix! I think the current patch fixes the original <31-digit
hang, but it still leaves an integer overflow in denom.

The patch keeps denom as a signed int:

int sign = 1, denom = 1, denom_digits = 0, part = 0;

and rejects only after more than 15 fractional digits:

if (++denom_digits > 15) {
    PyErr_SetString(DataError, "too many fractional digits");
    return NULL;
}
denom *= 10;

On common platforms where int is 32-bit, denom *= 10 overflows at the 10th
fractional digit:

1 digit  -> 10
6 digits -> 1000000
9 digits -> 1000000000
10 digits -> 10000000000  // overflows 32-bit int

So inputs with roughly 10-15 fractional digits can still execute signed integer
overflow before the new DataError path is reached. That may no longer trigger
the original infinite loop, but it still leaves undefined behavior and can
produce incorrect interval parsing.

A simple test case for this remaining issue is:

import psycopg2.extensions as ext

ext.INTERVAL(b"0:0:0." + b"0" * 12, None)

I would suggest guarding before multiplication, for example:

if (part == 6) {
    if (denom > INT_MAX / 10) {
        PyErr_SetString(DataError, "too many fractional digits");
        return NULL;
    }
    denom *= 10;
}

Alternatively, if the intended behavior is to allow up to 15 fractional digits,
denom should be widened to a 64-bit type such as PY_LONG_LONG, and the code
should still guard before multiplying.

Hope this helps! :)

[dvarrazzo]

Hmm, you are right, I picked 15 to "stay into a 16 bits signed int" but those are binary digits, not decimal digits.

Probably better to detect the overflow in the loop. I dropped exception raising. Dropping digits is enough.

[michaelknap]

Hmm, you are right, I picked 15 to "stay into a 16 bits signed int" but those are binary digits, not decimal digits.

Probably better to detect the overflow in the loop. I dropped exception raising. Dropping digits is enough.

Thank you for quick updates and I'm sorry for my late reply.

I think quietly truncating extra fractional digits is a reasonable behavior, but
this version still has a signed integer overflow issue.

The overflow happens here before the comparison can detect it:

int new_denom = denom * 10;

Because denom is a signed int, denom * 10 has undefined behavior once the
mathematical result exceeds INT_MAX. Checking new_denom >= denom after the
multiplication is too late.

The comparison also does not reliably detect wraparound. For example, on common
32-bit two's-complement behavior:

1000000000 * 10 -> 1410065408

The wrapped value is still greater than the old denominator, so the overflowed
value could be accepted.

If the desired behavior is to quietly truncate extra fractional digits, I think
the multiplication should be guarded before it happens:

if (part == 6) {
    if (denom <= INT_MAX / 10) {
        denom *= 10;
    }
}

This may require including `<limits.h> for INT_MAX.

[dvarrazzo]

@michaelknap you are right: I had assumed a two's complement but the behaviour is defined to be... undefined.

So checking for overflow as you suggest seems the right call.

Pushed and rebased.

[michaelknap]

Thank you for the changes and for all the work that goes into this module :) All the best.