|
From: Mike C. <ma...@mc...> - 2019-09-09 13:31:27
|
In glibc 5aad5f617892e75d91d4c8fb7594ff35b610c042 (first released in v2.28) a call to strncmp was added to dl-load.c:is_dst. This causes valgrind to complain about glibc's highly-optimised strncmp performing sixteen-byte reads on short strings in ld.so. Let's intercept strncmp in ld.so too so we use valgrind's simple version to avoid this problem. --- shared/vg_replace_strmem.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/shared/vg_replace_strmem.c b/shared/vg_replace_strmem.c index 87a4bcc55..5c05644fe 100644 --- a/shared/vg_replace_strmem.c +++ b/shared/vg_replace_strmem.c @@ -648,6 +648,8 @@ static inline void my_exit ( int x ) STRNCMP(VG_Z_LIBC_SONAME, __GI_strncmp) STRNCMP(VG_Z_LIBC_SONAME, __strncmp_sse2) STRNCMP(VG_Z_LIBC_SONAME, __strncmp_sse42) + STRNCMP(VG_Z_LD_LINUX_SO_2, strncmp) + STRNCMP(VG_Z_LD_LINUX_X86_64_SO_2, strncmp) #elif defined(VGO_darwin) STRNCMP(VG_Z_LIBC_SONAME, strncmp) -- 2.20.1 |
|
From: Mark W. <ma...@kl...> - 2019-09-10 11:26:30
|
Hi Mike, On Mon, 2019-09-09 at 14:16 +0100, Mike Crowe wrote: > In glibc 5aad5f617892e75d91d4c8fb7594ff35b610c042 (first released in > v2.28) a call to strncmp was added to dl-load.c:is_dst. This causes > valgrind to complain about glibc's highly-optimised strncmp performing > sixteen-byte reads on short strings in ld.so. Let's intercept strncmp in > ld.so too so we use valgrind's simple version to avoid this problem. > --- > shared/vg_replace_strmem.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/shared/vg_replace_strmem.c b/shared/vg_replace_strmem.c > index 87a4bcc55..5c05644fe 100644 > --- a/shared/vg_replace_strmem.c > +++ b/shared/vg_replace_strmem.c > @@ -648,6 +648,8 @@ static inline void my_exit ( int x ) > STRNCMP(VG_Z_LIBC_SONAME, __GI_strncmp) > STRNCMP(VG_Z_LIBC_SONAME, __strncmp_sse2) > STRNCMP(VG_Z_LIBC_SONAME, __strncmp_sse42) > + STRNCMP(VG_Z_LD_LINUX_SO_2, strncmp) > + STRNCMP(VG_Z_LD_LINUX_X86_64_SO_2, strncmp) > > #elif defined(VGO_darwin) > STRNCMP(VG_Z_LIBC_SONAME, strncmp) I think this correct, but two questions: - You add it for ld-linux.so.2 (the x86 dynamic linker) and ld-linux-x86-64.so.2 (the amd64 dynamic linker). Did you observe it on both arches? Even if you didn't it might be a good idea anyway, just in case. - How did you observe it? Do you have a replicator? Interested in the call chain, to make sure that this is enough, or whether we might need a "hard-wire" instead ld.so intercepts might be called "too early" in some cases, before we had a chance to setup the full intercept mechanism. (When is is_dst () called?) Thanks, Mark |
|
From: Mike C. <ma...@mc...> - 2019-09-10 17:02:58
|
On Tuesday 10 September 2019 at 13:26:20 +0200, Mark Wielaard wrote:
> Hi Mike,
>
> On Mon, 2019-09-09 at 14:16 +0100, Mike Crowe wrote:
> > In glibc 5aad5f617892e75d91d4c8fb7594ff35b610c042 (first released in
> > v2.28) a call to strncmp was added to dl-load.c:is_dst. This causes
> > valgrind to complain about glibc's highly-optimised strncmp performing
> > sixteen-byte reads on short strings in ld.so. Let's intercept strncmp in
> > ld.so too so we use valgrind's simple version to avoid this problem.
> > ---
> > shared/vg_replace_strmem.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/shared/vg_replace_strmem.c b/shared/vg_replace_strmem.c
> > index 87a4bcc55..5c05644fe 100644
> > --- a/shared/vg_replace_strmem.c
> > +++ b/shared/vg_replace_strmem.c
> > @@ -648,6 +648,8 @@ static inline void my_exit ( int x )
> > STRNCMP(VG_Z_LIBC_SONAME, __GI_strncmp)
> > STRNCMP(VG_Z_LIBC_SONAME, __strncmp_sse2)
> > STRNCMP(VG_Z_LIBC_SONAME, __strncmp_sse42)
> > + STRNCMP(VG_Z_LD_LINUX_SO_2, strncmp)
> > + STRNCMP(VG_Z_LD_LINUX_X86_64_SO_2, strncmp)
> >
> > #elif defined(VGO_darwin)
> > STRNCMP(VG_Z_LIBC_SONAME, strncmp)
>
> I think this correct, but two questions:
>
> - You add it for ld-linux.so.2 (the x86 dynamic linker)
> and ld-linux-x86-64.so.2 (the amd64 dynamic linker).
> Did you observe it on both arches? Even if you didn't
> it might be a good idea anyway, just in case.
I tested on x86-64 (with AVX) and core-i7 (with SSE4.2 I think) and could
reproduce the problem on both. The glibc code is used on all architectures,
but not all of them have an optimised strncmp that confuses valgrind. In
particular, mere i686 does not.
It looks like glibc also has optimised strncmp implementations for AArch64
and powerpc32/64 at least. I've not tested these (although I could probably
test AArch64 later this week.) So, it looks like I should probably add a
line for VG_Z_LD_LINUX_AARCH64_SO_1.
Lines for VG_Z_LD_LINUX_SO_3, VG_Z_LD64_SO_1 and VG_Z_LD_SO_1 may also be
necessary.
I can submit a new patch to add all of these if you agree.
> - How did you observe it? Do you have a replicator?
> Interested in the call chain, to make sure that this is
> enough, or whether we might need a "hard-wire" instead
> ld.so intercepts might be called "too early" in some
> cases, before we had a chance to setup the full intercept
> mechanism. (When is is_dst () called?)
It happened when iconv tried to dlopen a code page file. Here's my test
case:
--8<--
#include <iconv.h>
#include <stdio.h>
int main()
{
iconv_t descriptor = iconv_open("UCS-4BE", "euc-kr");
if (descriptor == (iconv_t)(-1)) {
fprintf(stderr, "Failed to get iconv\n");
return 1;
}
fprintf(stderr, "Got iconv\n");
return 0;
}
-->8--
(although I imagine that you only need the iconv_open line.)
Thanks.
Mike.
|
|
From: Mark W. <ma...@kl...> - 2019-09-12 09:21:24
|
Hi Mike,
On Tue, Sep 10, 2019 at 06:02:40PM +0100, Mike Crowe wrote:
> On Tuesday 10 September 2019 at 13:26:20 +0200, Mark Wielaard wrote:
> > On Mon, 2019-09-09 at 14:16 +0100, Mike Crowe wrote:
> > > In glibc 5aad5f617892e75d91d4c8fb7594ff35b610c042 (first released in
> > > v2.28) a call to strncmp was added to dl-load.c:is_dst. This causes
> > > valgrind to complain about glibc's highly-optimised strncmp performing
> > > sixteen-byte reads on short strings in ld.so. Let's intercept strncmp in
> > > ld.so too so we use valgrind's simple version to avoid this problem.
> > > ---
> > > shared/vg_replace_strmem.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/shared/vg_replace_strmem.c b/shared/vg_replace_strmem.c
> > > index 87a4bcc55..5c05644fe 100644
> > > --- a/shared/vg_replace_strmem.c
> > > +++ b/shared/vg_replace_strmem.c
> > > @@ -648,6 +648,8 @@ static inline void my_exit ( int x )
> > > STRNCMP(VG_Z_LIBC_SONAME, __GI_strncmp)
> > > STRNCMP(VG_Z_LIBC_SONAME, __strncmp_sse2)
> > > STRNCMP(VG_Z_LIBC_SONAME, __strncmp_sse42)
> > > + STRNCMP(VG_Z_LD_LINUX_SO_2, strncmp)
> > > + STRNCMP(VG_Z_LD_LINUX_X86_64_SO_2, strncmp)
> > >
> > > #elif defined(VGO_darwin)
> > > STRNCMP(VG_Z_LIBC_SONAME, strncmp)
> >
> > I think this correct, but two questions:
> >
> > - You add it for ld-linux.so.2 (the x86 dynamic linker)
> > and ld-linux-x86-64.so.2 (the amd64 dynamic linker).
> > Did you observe it on both arches? Even if you didn't
> > it might be a good idea anyway, just in case.
>
> I tested on x86-64 (with AVX) and core-i7 (with SSE4.2 I think) and could
> reproduce the problem on both. The glibc code is used on all architectures,
> but not all of them have an optimised strncmp that confuses valgrind. In
> particular, mere i686 does not.
>
> It looks like glibc also has optimised strncmp implementations for AArch64
> and powerpc32/64 at least. I've not tested these (although I could probably
> test AArch64 later this week.) So, it looks like I should probably add a
> line for VG_Z_LD_LINUX_AARCH64_SO_1.
>
> Lines for VG_Z_LD_LINUX_SO_3, VG_Z_LD64_SO_1 and VG_Z_LD_SO_1 may also be
> necessary.
>
> I can submit a new patch to add all of these if you agree.
Probably, yes, but even with your reproducer I have been unable to
replicate on any of my setups.
> > - How did you observe it? Do you have a replicator?
> > Interested in the call chain, to make sure that this is
> > enough, or whether we might need a "hard-wire" instead
> > ld.so intercepts might be called "too early" in some
> > cases, before we had a chance to setup the full intercept
> > mechanism. (When is is_dst () called?)
>
> It happened when iconv tried to dlopen a code page file. Here's my test
> case:
>
> --8<--
> #include <iconv.h>
> #include <stdio.h>
>
> int main()
> {
> iconv_t descriptor = iconv_open("UCS-4BE", "euc-kr");
> if (descriptor == (iconv_t)(-1)) {
> fprintf(stderr, "Failed to get iconv\n");
> return 1;
> }
>
> fprintf(stderr, "Got iconv\n");
>
> return 0;
> }
> -->8--
Would you be able to provide more information on your setup? I tried
with a Fedora x86_64 glibc 2.29 and Debian x86_64 glibc 2.28 (both
with avx and avx2), but couldn't trigger it. What does the
valgrind/memcheck error look like?
Thanks,
Mark
|
|
From: Mike C. <ma...@mc...> - 2019-09-13 11:07:58
|
On Thursday 12 September 2019 at 11:21:05 +0200, Mark Wielaard wrote: > On Tue, Sep 10, 2019 at 06:02:40PM +0100, Mike Crowe wrote: > > On Tuesday 10 September 2019 at 13:26:20 +0200, Mark Wielaard wrote: > > > On Mon, 2019-09-09 at 14:16 +0100, Mike Crowe wrote: > > > > In glibc 5aad5f617892e75d91d4c8fb7594ff35b610c042 (first released in > > > > v2.28) a call to strncmp was added to dl-load.c:is_dst. This causes > > > > valgrind to complain about glibc's highly-optimised strncmp performing > > > > sixteen-byte reads on short strings in ld.so. Let's intercept strncmp in > > > > ld.so too so we use valgrind's simple version to avoid this problem. > > > > --- > > > > shared/vg_replace_strmem.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/shared/vg_replace_strmem.c b/shared/vg_replace_strmem.c > > > > index 87a4bcc55..5c05644fe 100644 > > > > --- a/shared/vg_replace_strmem.c > > > > +++ b/shared/vg_replace_strmem.c > > > > @@ -648,6 +648,8 @@ static inline void my_exit ( int x ) > > > > STRNCMP(VG_Z_LIBC_SONAME, __GI_strncmp) > > > > STRNCMP(VG_Z_LIBC_SONAME, __strncmp_sse2) > > > > STRNCMP(VG_Z_LIBC_SONAME, __strncmp_sse42) > > > > + STRNCMP(VG_Z_LD_LINUX_SO_2, strncmp) > > > > + STRNCMP(VG_Z_LD_LINUX_X86_64_SO_2, strncmp) > > > > > > > > #elif defined(VGO_darwin) > > > > STRNCMP(VG_Z_LIBC_SONAME, strncmp) > > > > > > I think this correct, but two questions: > > > > > > - You add it for ld-linux.so.2 (the x86 dynamic linker) > > > and ld-linux-x86-64.so.2 (the amd64 dynamic linker). > > > Did you observe it on both arches? Even if you didn't > > > it might be a good idea anyway, just in case. > > > > I tested on x86-64 (with AVX) and core-i7 (with SSE4.2 I think) and could > > reproduce the problem on both. The glibc code is used on all architectures, > > but not all of them have an optimised strncmp that confuses valgrind. In > > particular, mere i686 does not. > > > > It looks like glibc also has optimised strncmp implementations for AArch64 > > and powerpc32/64 at least. I've not tested these (although I could probably > > test AArch64 later this week.) So, it looks like I should probably add a > > line for VG_Z_LD_LINUX_AARCH64_SO_1. > > > > Lines for VG_Z_LD_LINUX_SO_3, VG_Z_LD64_SO_1 and VG_Z_LD_SO_1 may also be > > necessary. > > > > I can submit a new patch to add all of these if you agree. > > Probably, yes, but even with your reproducer I have been unable to > replicate on any of my setups. I tested on AArch64 today with valgrind-3.15 and the fix in 89423f5d8ba05 to avoid complaints about stpcpy, but could not reproduce the failure there using iconv_open. It looks like AArch64 has an optimised implementation of strncmp that processes eight bytes at a time, which ought to be confusing memcheck but for some reason isn't in this case. Perhaps the strings aren't aligned well enough for the optimisation to be used? It doesn't look like ARM has an optimised strncmp. Will it do any harm to use valgrind's simplified version regardless on AArch64 just in case? Thanks. Mike. |
|
From: Mark W. <ma...@kl...> - 2019-09-12 09:40:40
|
On Thu, Sep 12, 2019 at 11:21:05AM +0200, Mark Wielaard wrote:
> > > - How did you observe it? Do you have a replicator?
> > > Interested in the call chain, to make sure that this is
> > > enough, or whether we might need a "hard-wire" instead
> > > ld.so intercepts might be called "too early" in some
> > > cases, before we had a chance to setup the full intercept
> > > mechanism. (When is is_dst () called?)
> >
> > It happened when iconv tried to dlopen a code page file. Here's my test
> > case:
> >
> > --8<--
> > #include <iconv.h>
> > #include <stdio.h>
> >
> > int main()
> > {
> > iconv_t descriptor = iconv_open("UCS-4BE", "euc-kr");
> > if (descriptor == (iconv_t)(-1)) {
> > fprintf(stderr, "Failed to get iconv\n");
> > return 1;
> > }
> >
> > fprintf(stderr, "Got iconv\n");
> >
> > return 0;
> > }
> > -->8--
>
> Would you be able to provide more information on your setup? I tried
> with a Fedora x86_64 glibc 2.29 and Debian x86_64 glibc 2.28 (both
> with avx and avx2), but couldn't trigger it. What does the
> valgrind/memcheck error look like?
O, I see what is going on.
In both cases, on my setup, the error is there, but suppressed.
There are various default suppresses in glibc-2.X.supp.in that cover
this. In particular:
{
dl-hack4-64bit-addr-1
Memcheck:Addr8
obj:*/lib*/ld-@GLIBC_VERSION@*.so*
obj:*/lib*/ld-@GLIBC_VERSION@*.so*
obj:*/lib*/ld-@GLIBC_VERSION@*.so*
}
When running with valgrind --default-suppressions=no I do get:
==27438== Invalid read of size 8
==27438== at 0x401DA70: strncmp (in /usr/lib64/ld-2.29.so)
==27438== by 0x4005D9D: is_dst (in /usr/lib64/ld-2.29.so)
==27438== by 0x40086C6: _dl_dst_count (in /usr/lib64/ld-2.29.so)
==27438== by 0x40088B7: expand_dynamic_string_token (in /usr/lib64/ld-2.29.so)
==27438== by 0x4008A21: fillin_rpath (in /usr/lib64/ld-2.29.so)
==27438== by 0x4008D33: decompose_rpath.isra.0 (in /usr/lib64/ld-2.29.so)
==27438== by 0x4009718: _dl_map_object (in /usr/lib64/ld-2.29.so)
==27438== by 0x400DC54: openaux (in /usr/lib64/ld-2.29.so)
==27438== by 0x49941F8: _dl_catch_exception (in /usr/lib64/libc-2.29.so)
==27438== by 0x400E09A: _dl_map_object_deps (in /usr/lib64/ld-2.29.so)
==27438== by 0x4013BF3: dl_open_worker (in /usr/lib64/ld-2.29.so)
==27438== by 0x49941F8: _dl_catch_exception (in /usr/lib64/libc-2.29.so)
I think your intercept is still a good idea. But it would be good to
see why you don't get the default suppression for this. And maybe
these default suppressions are really too broad and maybe should be
removed?
Cheers,
Mark
|
|
From: Mike C. <ma...@mc...> - 2019-09-12 11:00:58
|
On Thursday 12 September 2019 at 11:40:22 +0200, Mark Wielaard wrote:
> On Thu, Sep 12, 2019 at 11:21:05AM +0200, Mark Wielaard wrote:
> > > > - How did you observe it? Do you have a replicator?
> > > > Interested in the call chain, to make sure that this is
> > > > enough, or whether we might need a "hard-wire" instead
> > > > ld.so intercepts might be called "too early" in some
> > > > cases, before we had a chance to setup the full intercept
> > > > mechanism. (When is is_dst () called?)
> > >
> > > It happened when iconv tried to dlopen a code page file. Here's my test
> > > case:
> > >
> > > --8<--
> > > #include <iconv.h>
> > > #include <stdio.h>
> > >
> > > int main()
> > > {
> > > iconv_t descriptor = iconv_open("UCS-4BE", "euc-kr");
> > > if (descriptor == (iconv_t)(-1)) {
> > > fprintf(stderr, "Failed to get iconv\n");
> > > return 1;
> > > }
> > >
> > > fprintf(stderr, "Got iconv\n");
> > >
> > > return 0;
> > > }
> > > -->8--
[snip]
> > Would you be able to provide more information on your setup?
I initially ran into the problem with OpenEmbedded running our unit tests
"cross-compiled" to x86_64 in a separate rootfs. I then switched to a
Fedora 29 Docker image with a self-built glibc 2.30 hacked into it. I'd
forgotten that whilst I could easily reproduce the problem with Fedora's
valgrind, I could not when using my own valgrind built from source. After a
bit of digging I discovered that default.supp for the Fedora valgrind was
different to the one being used when I built from source. Sorry that I
forgot to mention that.
[snip]
> When running with valgrind --default-suppressions=no I do get:
>
> ==27438== Invalid read of size 8
> ==27438== at 0x401DA70: strncmp (in /usr/lib64/ld-2.29.so)
> ==27438== by 0x4005D9D: is_dst (in /usr/lib64/ld-2.29.so)
> ==27438== by 0x40086C6: _dl_dst_count (in /usr/lib64/ld-2.29.so)
> ==27438== by 0x40088B7: expand_dynamic_string_token (in /usr/lib64/ld-2.29.so)
> ==27438== by 0x4008A21: fillin_rpath (in /usr/lib64/ld-2.29.so)
> ==27438== by 0x4008D33: decompose_rpath.isra.0 (in /usr/lib64/ld-2.29.so)
> ==27438== by 0x4009718: _dl_map_object (in /usr/lib64/ld-2.29.so)
> ==27438== by 0x400DC54: openaux (in /usr/lib64/ld-2.29.so)
> ==27438== by 0x49941F8: _dl_catch_exception (in /usr/lib64/libc-2.29.so)
> ==27438== by 0x400E09A: _dl_map_object_deps (in /usr/lib64/ld-2.29.so)
> ==27438== by 0x4013BF3: dl_open_worker (in /usr/lib64/ld-2.29.so)
> ==27438== by 0x49941F8: _dl_catch_exception (in /usr/lib64/libc-2.29.so)
>
> I think your intercept is still a good idea. But it would be good to
> see why you don't get the default suppression for this. And maybe
> these default suppressions are really too broad and maybe should be
> removed?
I didn't investigate why the Fedora default.supp files end up being
different to the one used when I built from source, but it is certainly
confusing.
Thanks.
Mike.
|
|
From: Mark W. <ma...@so...> - 2022-05-13 22:55:45
|
https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=947388eb043ea1c44b37df94046e1eee790ad776 commit 947388eb043ea1c44b37df94046e1eee790ad776 Author: Mike Crowe <ma...@mc...> Date: Mon Sep 9 14:16:16 2019 +0100 Intercept strncmp for glibc ld.so v2.28+ In glibc 5aad5f617892e75d91d4c8fb7594ff35b610c042 (first released in v2.28) a call to strncmp was added to dl-load.c:is_dst. This causes valgrind to complain about glibc's highly-optimised strncmp performing sixteen-byte reads on short strings in ld.so. Let's intercept strncmp in ld.so too so we use valgrind's simple version to avoid this problem. Diff: --- NEWS | 1 + shared/vg_replace_strmem.c | 2 ++ 2 files changed, 3 insertions(+) diff --git a/NEWS b/NEWS index dac9a1ce2a..4bf76608b7 100644 --- a/NEWS +++ b/NEWS @@ -35,6 +35,7 @@ bugzilla (https://bugs.kde.org/enter_bug.cgi?product=valgrind) rather than mailing the developers (or mailing lists) directly -- bugs that are not entered into bugzilla tend to get forgotten about or ignored. +434764 iconv_open causes ld.so v2.28+ to use optimised strncmp 446754 Improve error codes from alloc functions under memcheck 452274 memcheck crashes with Assertion 'sci->status.what == SsIdle' failed 452779 Valgrind fails to build on FreeBSD 13.0 with llvm-devel (15.0.0) diff --git a/shared/vg_replace_strmem.c b/shared/vg_replace_strmem.c index 3b42b3a871..5396e83be0 100644 --- a/shared/vg_replace_strmem.c +++ b/shared/vg_replace_strmem.c @@ -710,6 +710,8 @@ static inline void my_exit ( int x ) STRNCMP(VG_Z_LIBC_SONAME, __GI_strncmp) STRNCMP(VG_Z_LIBC_SONAME, __strncmp_sse2) STRNCMP(VG_Z_LIBC_SONAME, __strncmp_sse42) + STRNCMP(VG_Z_LD_LINUX_SO_2, strncmp) + STRNCMP(VG_Z_LD_LINUX_X86_64_SO_2, strncmp) #elif defined(VGO_freebsd) STRNCMP(VG_Z_LIBC_SONAME, strncmp) |