Re: Fw:Re: Fw: ltree_compare in contrib/ltree/ltree_op.c overflows int32 on deep ltree comparisons, returning the wrong sign
Re: Fw:Re: Fw: ltree_compare in contrib/ltree/ltree_op.c overflows int32 on deep ltree comparisons, returning the wrong sign
От:
Heikki Linnakangas <hlinnaka@iki.fi>
Дата:
On 15/06/2026 18:24, Ayush Tiwari wrote: > On Mon, 15 Jun 2026 at 20:38, Heikki Linnakangas > wrote: > > On 13/06/2026 09:12, Ayush Tiwari wrote: > > This looks like a classic case of integer overflow that's > > happening in ltree_compare function in ltree_op.c. > > > > return (al->len - bl->len) * 10 * (an + 1); > > return res * 10 * (an + 1); > > return (a->numlevel - b->numlevel) * 10 * (an + 1); > > > > I think the calculation should be done as int64, something of > this sort: > > Yeah, that works. However, I note that the multiplication is only > really > needed by the ltree_penalty() caller. All the other callers just check > if the return value is less than, equal, or greater than zero. It feels > a little silly to do all that work of multiplication and clamping for > those callers. And for ltree_penalty(), the caller actually converts > the > return value to a float, so clamping it to int32 range feels a little > silly for that too. So I propose the attached, which splits the > ltree_compare() function into two variants: one for ltree_penalty() > that > returns a float, and one for others that don't care about the > "magnitude". It duplicates a little code, but I think it's easier to > reason about. What do you think? > > > I had thought initially of using the method you have added, > (not with float return-type though, I thought of planning to create > a duplicate function with int64 type just for ltree_penalty(), but float > type > is better) noting the same thing that rest callers just care about > comparison with 0. > > But thought backpatching it would be harder, hence I used the int64 > method. However, your patch looks much better than the ugly > behaviour and I agree it did not make sense to do those > multiplications at all other comparison functions. Great, committed. Thanks! - Heikki
Re: Fw:Re: Fw: ltree_compare in contrib/ltree/ltree_op.c overflows int32 on deep ltree comparisons, returning the wrong sign
От:
Heikki Linnakangas <hlinnaka@iki.fi>
Дата:
On 13/06/2026 09:12, Ayush Tiwari wrote: > This looks like a classic case of integer overflow that's > happening in ltree_compare function in ltree_op.c. > > return (al->len - bl->len) * 10 * (an + 1); > return res * 10 * (an + 1); > return (a->numlevel - b->numlevel) * 10 * (an + 1); > > I think the calculation should be done as int64, something of this sort: > > int64 v = (int64) (al->len - bl->len) * 10 * (an + 1); > if (v > PG_INT32_MAX) return PG_INT32_MAX; > if (v < PG_INT32_MIN) return PG_INT32_MIN; > return (int) v; > > And needed to adjust the ltree_penalty function too. > > Attached is a draft patch for this, I guess we can add a helper > function too for the above conversion. Yeah, that works. However, I note that the multiplication is only really needed by the ltree_penalty() caller. All the other callers just check if the return value is less than, equal, or greater than zero. It feels a little silly to do all that work of multiplication and clamping for those callers. And for ltree_penalty(), the caller actually converts the return value to a float, so clamping it to int32 range feels a little silly for that too. So I propose the attached, which splits the ltree_compare() function into two variants: one for ltree_penalty() that returns a float, and one for others that don't care about the "magnitude". It duplicates a little code, but I think it's easier to reason about. What do you think? - Heikki
Fw:Re: Fw: ltree_compare in contrib/ltree/ltree_op.c overflows int32 on deep ltree comparisons, returning the wrong sign
От:
王跃林 <violin0613@tju.edu.cn>
Дата:
-------- 转发邮件信息 -------- 发件人:Noah Misch <noah@leadboat.com> 发送日期:2026-06-13 08:31:47 收件人:"violin0613@tju.edu.cn" <violin0613@tju.edu.cn> 抄送人:security@postgresql.org 主题:Re: Fw: ltree_compare in contrib/ltree/ltree_op.c overflows int32 on deep ltree comparisons, returning the wrong sign On Mon, May 25, 2026 at 10:58:05PM +0800, violin0613@tju.edu.cn wrote: > PoC > > File vuln_001.sql > > CREATE EXTENSION IF NOT EXISTS ltree; > â > WITH s AS (SELECT 'a'::ltree AS v), > l AS (SELECT (repeat('a.', 19999) || 'a')::ltree AS v) > SELECT (l.v > s.v) AS long_gt_short_expected_true, > (l.v < s.v) AS long_lt_short_expected_false > FROM s, l; > > Process > > psql -h /tmp -p 36265 -U postgres -f vuln_001.sql > > Results > > long_gt_short_expected_true | long_lt_short_expected_false > -----------------------------+------------------------------ > f | t > > Both columns are inverted. long > short returned false, long < short > returned true. This is certainly a bug, but it's not a vuln. Please report the bug to pgsql-bugs@postgresql.org.
Re: Fw:Re: Fw: ltree_compare in contrib/ltree/ltree_op.c overflows int32 on deep ltree comparisons, returning the wrong sign
От:
Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Дата:
Hi,
On Sat, 13 Jun 2026 at 09:20, 王跃林 <violin0613@tju.edu.cn> wrote:
主题:Re: Fw: ltree_compare in contrib/ltree/ltree_op.c overflows int32 on deep ltree comparisons, returning the wrong sign On Mon, May 25, 2026 at 10:58:05PM +0800, violin0613@tju.edu.cn wrote: > PoC > > File vuln_001.sql > > CREATE EXTENSION IF NOT EXISTS ltree; > â > WITH s AS (SELECT 'a'::ltree AS v), > l AS (SELECT (repeat('a.', 19999) || 'a')::ltree AS v) > SELECT (l.v > s.v) AS long_gt_short_expected_true, > (l.v < s.v) AS long_lt_short_expected_false > FROM s, l; > > Process > > psql -h /tmp -p 36265 -U postgres -f vuln_001.sql > > Results > > long_gt_short_expected_true | long_lt_short_expected_false > -----------------------------+------------------------------ > f | t > > Both columns are inverted. long > short returned false, long < short > returned true.
This looks like a classic case of integer overflow that's
happening in ltree_compare function in ltree_op.c.
return (al->len - bl->len) * 10 * (an + 1);
return res * 10 * (an + 1);
return (a->numlevel - b->numlevel) * 10 * (an + 1);
I think the calculation should be done as int64, something of this sort:
int64 v = (int64) (al->len - bl->len) * 10 * (an + 1);
if (v > PG_INT32_MAX) return PG_INT32_MAX;
if (v < PG_INT32_MIN) return PG_INT32_MIN;
return (int) v;
And needed to adjust the ltree_penalty function too.
Attached is a draft patch for this, I guess we can add a helper
return (al->len - bl->len) * 10 * (an + 1);
return res * 10 * (an + 1);
return (a->numlevel - b->numlevel) * 10 * (an + 1);
I think the calculation should be done as int64, something of this sort:
int64 v = (int64) (al->len - bl->len) * 10 * (an + 1);
if (v > PG_INT32_MAX) return PG_INT32_MAX;
if (v < PG_INT32_MIN) return PG_INT32_MIN;
return (int) v;
And needed to adjust the ltree_penalty function too.
Attached is a draft patch for this, I guess we can add a helper
function too for the above conversion.
Thoughts?
Regards,
Ayush
Thoughts?
Regards,
Ayush
Re: Fw:Re: Fw: ltree_compare in contrib/ltree/ltree_op.c overflows int32 on deep ltree comparisons, returning the wrong sign
От:
Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Дата:
Hi,
On Mon, 15 Jun 2026 at 20:38, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 13/06/2026 09:12, Ayush Tiwari wrote:
> This looks like a classic case of integer overflow that's
> happening in ltree_compare function in ltree_op.c.
>
> return (al->len - bl->len) * 10 * (an + 1);
> return res * 10 * (an + 1);
> return (a->numlevel - b->numlevel) * 10 * (an + 1);
>
> I think the calculation should be done as int64, something of this sort:
Yeah, that works. However, I note that the multiplication is only really
needed by the ltree_penalty() caller. All the other callers just check
if the return value is less than, equal, or greater than zero. It feels
a little silly to do all that work of multiplication and clamping for
those callers. And for ltree_penalty(), the caller actually converts the
return value to a float, so clamping it to int32 range feels a little
silly for that too. So I propose the attached, which splits the
ltree_compare() function into two variants: one for ltree_penalty() that
returns a float, and one for others that don't care about the
"magnitude". It duplicates a little code, but I think it's easier to
reason about. What do you think?
I had thought initially of using the method you have added,
(not with float return-type though, I thought of planning to create
a duplicate function with int64 type just for ltree_penalty(), but float type
is better) noting the same thing that rest callers just care about
comparison with 0.
But thought backpatching it would be harder, hence I used the int64
method. However, your patch looks much better than the ugly
behaviour and I agree it did not make sense to do those
multiplications at all other comparison functions.
Regards,
Ayush