Date: Mon, 12 Feb 2024 17:04:23 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Mats Kindahl <mats@timescale.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Thomas Munro <thomas.munro@gmail.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: glibc qsort() vulnerability
Message-ID: <20240212230423.GA3519@nathanxps13>
References: 
 <CA+14427pVjvgnVGiyM45e_EyKgCmzgRiK3dSQJqfOtHAnY8Maw@mail.gmail.com>
 <CA+14427QfHELBNskJKPHA96yOJ6aUZE_XqbFBC7hz+hmywTtPQ@mail.gmail.com>
 <20240209200828.GB665650@nathanxps13>
 <CA+14425kn0RxC62M7ZaD5BRzBJEPRRLQQB4DGdL+=vxHS1E81Q@mail.gmail.com>
 <20240210205332.GA1124797@nathanxps13>
 <CA+14427j_ExhaicH96i3k8OnZaHYkYQtPm-Q66BzY96W3qBJ-A@mail.gmail.com>
 <20240212155715.GB1645880@nathanxps13>
 <CA+14426fK=NNECQDo7cmRHi5CfhRQcUqprfrzhat9MK2dYXK+A@mail.gmail.com>
 <20240212205138.GA1815383@nathanxps13>
 <20240212213130.jp5vwotwazypaaez@awork3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240212213130.jp5vwotwazypaaez@awork3.anarazel.de>
Archived-At: 
 <https://www.postgresql.org/message-id/20240212230423.GA3519%40nathanxps13>
Precedence: bulk

On Mon, Feb 12, 2024 at 01:31:30PM -0800, Andres Freund wrote:
> One thing that's worth checking is if this ends up with *worse* code when the
> comparators are inlined. I think none of the changed comparators will end up
> getting used with an inlined sort, but ...

Yeah, AFAICT the only inlined sorts are in tuplesort.c and bufmgr.c, and
the patches don't touch those files.

> The reason we could end up with worse code is that when inlining the
> comparisons would make less sense for the compiler. Consider e.g.
> 	return DO_COMPARE(a, b) < 0 ?
> 		(DO_COMPARE(b, c) < 0 ? b : (DO_COMPARE(a, c) < 0 ? c : a))
> 		: (DO_COMPARE(b, c) > 0 ? b : (DO_COMPARE(a, c) < 0 ? a : c));
> 
> With a naive implementation the compiler will understand it only cares about
> a < b, not about the other possibilities. I'm not sure that's still true with
> the more complicated optimized version.

You aren't kidding [0].  Besides perhaps adding a comment in
sort_template.h, is there anything else you think we should do about this
now?

[0] https://godbolt.org/z/bbTqK54zK

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com