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>
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Cc: 王跃林 <violin0613@tju.edu.cn>, pgsql-bugs@postgresql.org
Date: 2026-06-16T06:37:42Z
Lists: pgsql-bugs
On 15/06/2026 18:24, Ayush Tiwari wrote: > On Mon, 15 Jun 2026 at 20:38, Heikki Linnakangas <hlinnaka@iki.fi > <mailto: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. Great, committed. Thanks! - Heikki
Commits
-
Fix int32 overflow in ltree_compare()
- f528a5606a83 14 (unreleased) landed
- 1bec6b1c146c 15 (unreleased) landed
- aca944e33786 16 (unreleased) landed
- c391c00d9dd7 17 (unreleased) landed
- c3e36a9a5f19 18 (unreleased) landed
- 3f3280491838 19 (unreleased) landed