Message-ID: <cc9b0272-849a-4a88-8200-59786d18b7a1@iki.fi>
Date: Wed, 7 Feb 2024 20:46:56 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: glibc qsort() vulnerability
To: Mats Kindahl <mats@timescale.com>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Nathan Bossart <nathandbossart@gmail.com>,
 pgsql-hackers@lists.postgresql.org
References: <CA+14426g2Wa9QuUpmakwPxXFWG_1FaY0AsApkvcTBy-YfS6uaw@mail.gmail.com>
 <508821.1707232276@sss.pgh.pa.us> <20240206205305.GB3891538@nathanxps13>
 <642084.1707252958@sss.pgh.pa.us>
 <CA+14425TqyUZ3-O7zYsW6u85yY5diCz-p_TPQ2bZovn-N-fQ9A@mail.gmail.com>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <CA+14425TqyUZ3-O7zYsW6u85yY5diCz-p_TPQ2bZovn-N-fQ9A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/cc9b0272-849a-4a88-8200-59786d18b7a1%40iki.fi>
Precedence: bulk

On 07/02/2024 11:09, Mats Kindahl wrote:
> On Tue, Feb 6, 2024 at 9:56 PM Tom Lane <tgl@sss.pgh.pa.us 
> <mailto:tgl@sss.pgh.pa.us>> wrote:
> 
>   Nathan Bossart <nathandbossart@gmail.com
>     <mailto:nathandbossart@gmail.com>> writes:
>      > Even if the glibc issue doesn't apply to Postgres, I'm tempted to
>     suggest
>      > that we make it project policy that comparison functions must be
>      > transitive.  There might be no real issues today, but if we write all
>      > comparison functions the way Mats is suggesting, it should be
>     easier to
>      > reason about overflow risks.
> 
>     A comparison routine that is not is probably broken, agreed.
>     I didn't look through the details of the patch --- I was more
>     curious whether we had a version of the qsort bug, because
>     if we do, we should fix that too.
> 
> The patch basically removes the risk of overflow in three routines and 
> just returns -1, 0, or 1, and adds a comment in one.
> 
> The routines modified do a subtraction of int:s and return that, which 
> can cause an overflow. This method is used for some int16 as well but 
> since standard conversions in C will perform the arithmetics in "int" 
> precision, this cannot overflow, so added a comment there. It might 
> still be a good idea to follow the same pattern for the int16 routines, 
> but since there is no bug there, I did not add them to the patch.

Doesn't hurt to fix the comparison functions, and +1 on using the same 
pattern everywhere.

However, we use our qsort() with user-defined comparison functions, and 
we cannot make any guarantees about what they might do. So we must 
ensure that our qsort() doesn't overflow, no matter what the comparison 
function does.

Looking at our ST_SORT(), it seems safe to me.

-- 
Heikki Linnakangas
Neon (https://neon.tech)