Re: [nft PATCH v2 02/11] mergesort: Fix sorting of string values

[Phil Sutter <phil@nwl.cc>]

On Wed, Jan 28, 2026 at 12:28:16AM +0100, Pablo Neira Ayuso wrote:
> On Fri, Nov 14, 2025 at 01:25:33AM +0100, Phil Sutter wrote:
> > Sorting order was obviously wrong, e.g. "ppp0" ordered before "eth1".
> > Moreover, this happened on Little Endian only so sorting order actually
> > depended on host's byteorder. By reimporting string values as Big
> > Endian, both issues are fixed: On one hand, GMP-internal byteorder no
> > longer depends on host's byteorder, on the other comparing strings
> > really starts with the first character, not the last.
> > 
> > Fixes: 14ee0a979b622 ("src: sort set elements in netlink_get_setelems()")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  src/mergesort.c                               |  7 +++
> >  tests/py/any/meta.t.json.output               | 54 -------------------
> >  tests/py/any/queue.t.json.output              |  4 +-
> >  tests/py/inet/osf.t.json.output               | 54 +++++++++++++++++++
> >  .../testcases/maps/dumps/0012map_0.json-nft   | 20 +++----
> >  .../shell/testcases/maps/dumps/0012map_0.nft  |  8 +--
> >  .../maps/dumps/named_ct_objects.json-nft      |  4 +-
> >  .../testcases/maps/dumps/named_ct_objects.nft |  4 +-
> >  .../sets/dumps/sets_with_ifnames.json-nft     |  4 +-
> >  .../sets/dumps/sets_with_ifnames.nft          |  2 +-
> >  10 files changed, 84 insertions(+), 77 deletions(-)
> > 
> > diff --git a/src/mergesort.c b/src/mergesort.c
> > index a9cba614612ed..97e36917280f3 100644
> > --- a/src/mergesort.c
> > +++ b/src/mergesort.c
> > @@ -37,6 +37,13 @@ static mpz_srcptr expr_msort_value(const struct expr *expr, mpz_t value)
> >  	case EXPR_RANGE:
> >  		return expr_msort_value(expr->left, value);
> >  	case EXPR_VALUE:
> > +		if (expr_basetype(expr)->type == TYPE_STRING) {
> > +			char buf[expr->len];
> > +
> > +			mpz_export_data(buf, expr->value, BYTEORDER_HOST_ENDIAN, expr->len);
> > +			mpz_import_data(value, buf, BYTEORDER_BIG_ENDIAN, expr->len);
> > +			return value;
> > +		}
> 
> This is also used for automerge, not only get_setelems().
> 
> Are you sure this is correct?

If it isn't, we don't have test coverage for the broken code-path.

Cheers, Phil