Re: glibc qsort() vulnerability

On Thu, Feb 8, 2024 at 9:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Nathan Bossart <nathandbossart@gmail.com> writes:
> > On Thu, Feb 08, 2024 at 11:59:54AM -0800, Andres Freund wrote:
> >> I'd put these static inlines into common/int.h. I don't think this is
> common
> >> enough to warrant being in c.h. Probably also doesn't hurt to have a
> not quite
> >> as generic name as INT_CMP, I'd not be too surprised if that's defined
> in some
> >> library.
> >>
> >> I think it's worth following int.h's pattern of including
> [s]igned/[u]nsigned
> >> in the name, an efficient implementation for signed might not be the
> same as
> >> for unsigned. And if we use static inlines, we need to do so for correct
> >> semantics anyway.
>
> > Seems reasonable to me.
>
> +1 here also.
>

Here is a new version introducing pg_cmp_s32 and friends and use them
instead of the INT_CMP macro introduced before. It also moves the
definitions to common/int.h and adds that as an include to all locations
using these functions.

Note that for integers with sizes less than sizeof(int), C standard
conversions will convert the values to "int" before doing the arithmetic,
so no casting is *necessary*. I did not force the 16-bit functions to
return -1 or 1 and have updated the comment accordingly.

The types "int" and "size_t" are treated as s32 and u32 respectively since
that seems to be the case for most of the code, even if strictly not
correct (size_t can be an unsigned long int for some architecture).

I also noted that in many situations size_t values are treated as "int" so
there is an overflow risk here, even if small. For example, the result of
"list_length" is assigned to an integer. I do not think this is an
immediate concern, but just wanted to mention it.

Best wishes,
Mats Kindahl

>
>                         regards, tom lane
>