Bug 397083

Created attachment 114265 [details]
Valgrind output.

I'm still seeing this on Ubuntu 22.04, with valgrind 3.18.1.
===================== foo.c ========================
#include <stdlib.h>
#include <wchar.h>

int main ()
{
  wchar_t *s = (wchar_t *) malloc (8 * sizeof (wchar_t));
  s[0] = '-';
  s[1] = 'N';
  s[2] = 'A';
  s[3] = 'N';
  s[4] = ' ';
  s[5] = '3';
  s[6] = '3';
  s[7] = '\0';
  return wmemcmp (s + 1, L"NAN", 3) == 0;
}
===================================================
$ gcc -ggdb foo.c
$ valgrind ./a.out
==2109475== Memcheck, a memory error detector
==2109475== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==2109475== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==2109475== Command: ./a.out
==2109475== 
==2109475== Invalid read of size 32
==2109475==    at 0x4A1C55D: __wmemcmp_avx2_movbe (memcmp-avx2-movbe.S:413)
==2109475==    by 0x10920D: main (foo.c:15)
==2109475==  Address 0x4aa6044 is 4 bytes inside a block of size 32 alloc'd
==2109475==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2109475==    by 0x10917E: main (foo.c:6)
==2109475== 
...

Should be a simple fix. I’ll have a go tonight or over the weekend.

A bit more to it than I'd hoped - we don't have a wmemcmp wrapper yet.

Created attachment 158110 [details]
first stab

Not tested yet

On gcc14 (Debian bookworm) doesn't generate errors with the testw example

The C example calling wmemcmp directly produces this

==660430== Invalid read of size 32
==660430==    at 0x49BD53D: __wmemcmp_avx2_movbe (memcmp-avx2-movbe.S:415)
==660430==    by 0x1091E9: main (wmemcmp.c:15)
==660430==  Address 0x4a47044 is 4 bytes inside a block of size 32 alloc'd
==660430==    at 0x48407B4: malloc (vg_replace_malloc.c:381)
==660430==    by 0x10915A: main (wmemcmp.c:6)

No error with the patch

(In reply to Paul Floyd from comment #5)
> Created attachment 158110 [details]

There's a small issue here: ISO C allows all possible wchar_t values to occur in the arrays passed to wmemcmp(), and requires that the comparison be done w.r.t. the order in the underlying integer type 'wchar_t'. For more on this topic, please see https://www.openwall.com/lists/musl/2023/04/18/5 and https://lists.gnu.org/archive/html/bug-gnulib/2023-04/msg00145.html .

Regarding your patch, this means that the line
         Int res = a0 - b0; \
should be replaced with something that does not assume that a0 and b0 are in the range 0..INT_MAX. For example,
         Int res = (a0 > b0) - (a0 < b0);
(This expression on the right-hand side has the noce property that it does not produce conditional jumps with GCC. See https://lists.gnu.org/archive/html/bug-gnulib/2020-07/msg00109.html .)

Additionally, watch out: wchar_t is signed (i.e. equivalent to int32_t) on most platforms. However it is unsigned (i.e. equivalent to uint32_t) on arm64 platforms (on glibc, musl libc, FreeBSD, but not on macOS).

It was just a copy and quick adaptation of the memchr representation. There are other issues - the byte count isn't right either.

There's a lot less benefit to using a loop of UWord steps since UWord is only 2x the size of wchar_t (whilst for memchr it is 8x the size of char).

Looking at a couple of libc implementations, FreeBSD keeps it very simple, no real effort to optimize.

wmemcmp(const wchar_t *s1, const wchar_t *s2, size_t n)
{
        size_t i;

        for (i = 0; i < n; i++) {
                if (*s1 != *s2) {
                        /* wchar might be unsigned */
                        return *s1 > *s2 ? 1 : -1;
                }
                s1++;
                s2++;
        }
        return 0;
}

GNU libc does try harder - though that also has to work on Windows where wchar_t is 16bits where I expect there will be more benefit.

Code is a bit longer so here's the link. In short, an unrolled loop doing 4 whar_ts at a time followed by 3 ifs to handle the remainder.

https://elixir.bootlin.com/glibc/glibc-2.37/source/wcsmbs/wmemcmp.c#L25

On RHEL 7.9 I just see

00000000000a8c40 T wmemchr

but for wmemcmp

00000000000b44b0 t __wmemcmp_sse2
000000000016b820 t __wmemcmp_sse4_1
000000000016f280 t __wmemcmp_ssse3
00000000000a8cc0 i wmemcmp

On RHEL 9.1 that becomes

00000000000f77d0 i __wmemchr
00000000000c0f80 t __wmemchr_avx2
00000000000c98e0 t __wmemchr_avx2_rtm
00000000000d2740 t __wmemchr_evex
00000000000d3240 t __wmemchr_evex_rtm
00000000000f77d0 t __wmemchr_ifunc
000000000003dfc0 t __wmemchr_sse2
00000000000f77d0 i wmemchr

and

00000000000f7860 i __wmemcmp
00000000000c1240 t __wmemcmp_avx2_movbe
00000000000c9bc0 t __wmemcmp_avx2_movbe_rtm
00000000000d2a40 t __wmemcmp_evex_movbe
00000000000f7860 t __wmemcmp_ifunc
00000000001075d0 t __wmemcmp_sse2
00000000000d4fe0 t __wmemcmp_ssse3
00000000000f7860 i wmemcmp

(ignoring a few GI versions)

I can replicate the issue with the C example, but not the C++ one on gcc-12.2.1-4.fc37.x86_64 glibc-2.36-9.fc37.x86_64

It looks like the original c++ issue (which called down to glibc wmemchr) has been fixed by bug #388862 in 3.14.0 (the reporter used 3.13.0). I think just intercepting the generic wmemchr (ifunc) is enough, you don't have to also intercept the internal variants.

(In reply to Mark Wielaard from comment #11)
> It looks like the original c++ issue (which called down to glibc wmemchr)
> has been fixed by bug #388862 in 3.14.0 (the reporter used 3.13.0). I think
> just intercepting the generic wmemchr (ifunc) is enough, you don't have to
> also intercept the internal variants.

And I think the same holds for wmemcmp. So you should just have to intercept the main wmemcmp function, not any of its variants (unless those are somehow internally called directly instead of going through the ifunc).

I left wmemcmp alone and just added a simple wmemchr wrapper, Linux only.

commit a2af9adec4536c3ce95d4ea5ddf15d00d7c55c6c (HEAD -> master, origin/master, origin/HEAD, wmemcmp)
Author: Paul Floyd <pjfloyd@wanadoo.fr>
Date:   Thu Apr 20 22:11:31 2023 +0200

    Bug 397083 - Likely false positive "uninitialised value(s)" for __wmemchr_avx2 and __wmemcmp_avx2_movbe

(In reply to Paul Floyd from comment #13)
> I left wmemcmp alone and just added a simple wmemchr wrapper, Linux only.

Actually, looking at your commit, you left wmemchr alone and added a simple wmemcmp wrapper.

Thanks! That should solve my issue.

I think this got fixed back in April 2023

*** Bug 452072 has been marked as a duplicate of this bug. ***