Hi Hackers, right(text, int) gets one input wrong:
SELECT right('abcdef', (-2147483648)::int4); -- 'abcdef'
SELECT right('abcdef', -2147483647); -- '' (correct)
SELECT right('abcdef', -3); -- 'def' (correct)
A negative n means "return all but the first |n| characters", so |n| that
big has to give an empty string. Only this one value is affected, and
left() is fine.
text_right() negates n to get the number of characters to skip:
if (n < 0)
n = -n;
else
n = pg_mbstrlen_with_len(p, len) - n;
off = pg_mbcharcliplen(p, len, n);
Negating PG_INT32_MIN overflows; under -fwrapv it comes back as
PG_INT32_MIN, still negative, and pg_mbcharcliplen() returns 0 for any
negative limit, so off is 0 and the whole string is returned.
The attached patch clamps to PG_INT32_MAX rather than negating. Any n
whose absolute value reaches the string's length skips all of it, and a
text value can't be longer than PG_INT32_MAX, so the answer is unchanged
for every other input. I did not make it an error, unlike
text_format_string_conversion() a few hundred lines down, which rejects a
width of INT_MIN (73e7025bd8e, complete with a "-INT_MIN is undefined"
comment): a format width has no sensible clamp, whereas an oversized skip
count does.
left() is not affected because its negative case adds n to the character
length instead of negating it, and since the length is non-negative and
bounded by the varlena size limit, that sum cannot overflow.
This dates to 49b27ab5514, which added left()/right() in 2010, and the
line has not been touched since; I could not find a previous report. It's
the same shape as b4dfae2ffac (money, INT64_MIN / -1) from a few weeks
ago.
The patch adds the case to the existing left()/right() test in text.sql,
which currently covers only generate_series(-5, 5). make check passes;
before adding the expected output I ran the suite deliberately and the
only difference was the new line.
--
Regards,
Ewan Young
v1-0001-Fix-right-with-the-most-negative-integer.patch
Description: Binary data
