Re: [PATCH] Avoid dlopen on debuginfod-client when not configured
Hi Dirk,
On Mon, Mar 28, 2022 at 11:36:41PM +0200, Dirk Müller wrote:
> Loading debuginfod-client is expensive, especially when
> FIPS is enabled, as it goes through time intensive self-checks
> on load.
I assume this is because debuginfod-client loads and initializes
libcurl, which triggers crypto checks for https support?
> Avoid dlopen() when no debuginfo url is set.
The patch itself looks right. But I am slightly afraid this
(theoretically?) will break some programs which set DEBUGINFOD_URLS
themselves. We currently have no other way to make libdwfl use
debuginfod-client. Thoughts?
Cheers,
Mark
> Signed-off-by: Dirk Müller
> ---
> libdwfl/ChangeLog | 5 +
> libdwfl/debuginfod-client.c | 12 +++-
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
> index 9c5c8517..1ec13f30 100644
> --- a/libdwfl/ChangeLog
> +++ b/libdwfl/ChangeLog
> @@ -1,3 +1,8 @@
> +2022-03-28 Dirk Müller
> +
> + * debuginfod-client.c (__libdwfl_debuginfod_init): Skip dlopen
> + if debuginfod url is unset.
> +
> 2022-02-18 Mark Wielaard
>
> * image-header.c (__libdw_image_header): Assign header values for
> diff --git a/libdwfl/debuginfod-client.c b/libdwfl/debuginfod-client.c
> index 99b66b6e..af9d1363 100644
> --- a/libdwfl/debuginfod-client.c
> +++ b/libdwfl/debuginfod-client.c
> @@ -101,7 +101,17 @@ __libdwfl_debuginfod_end (debuginfod_client *c)
> void __attribute__ ((constructor))
> __libdwfl_debuginfod_init (void)
> {
> - void *debuginfod_so = dlopen(DEBUGINFOD_SONAME, RTLD_LAZY);
> + void *debuginfod_so;
> +
> + /* Is there any server we can query? If not, don't do any work,
> + just return with ENOSYS. Don't even access the cache. */
> + char *urls_envvar = getenv(DEBUGINFOD_URLS_ENV_VAR);
> + if (urls_envvar == NULL || urls_envvar[0] == '\0')
> +{
> + return;
> +}
> +
> + debuginfod_so = dlopen(DEBUGINFOD_SONAME, RTLD_LAZY);
>
>if (debuginfod_so != NULL)
> {
> --
> 2.35.1
>
Re: [PATCH] Avoid dlopen on debuginfod-client when not configured
Hi - > > Avoid dlopen() when no debuginfo url is set. > > The patch itself looks right. But I am slightly afraid this > (theoretically?) will break some programs which set DEBUGINFOD_URLS > themselves. We currently have no other way to make libdwfl use > debuginfod-client. Thoughts? I've been thinking that doing all this dlopen work in the real dwfl function(s) rather than the solib ctor would be fine. - FChE
Re: [PATCH] Avoid dlopen on debuginfod-client when not configured
Hi Mark, Am Di., 29. März 2022 um 09:42 Uhr schrieb Mark Wielaard : > I assume this is because debuginfod-client loads and initializes > libcurl, which triggers crypto checks for https support? Yes, the dependencies of the DSO contain libcurl and other libraries including the openssl library, which, when patched with FIPS support, take quite a while to initialize. > The patch itself looks right. But I am slightly afraid this > (theoretically?) will break some programs which set DEBUGINFOD_URLS > themselves. We currently have no other way to make libdwfl use > debuginfod-client. Thoughts? I was concerned about that as well, but I couldn't google a case where this is the case. What we can also do is to do the initialization lazily upon first use rather than in a constructor function, but the intention of initializing early has been specified as avoiding issues in multiple threads with libcurl: commit fa0226a78a101d26fd80c7e9e70a5f0aa6f9d1cc Author: Mark Wielaard Date: Sun Nov 17 22:17:26 2019 +0100 debuginfod: add client context Add a mandatory debuginfod_begin()/_end() call pair to manage a client object that represents persistent but non-global state. From libdwfl, dlopen the debuginfod.so client library early on. This hopefully makes sure that the code (and the libcurl.so dependency) is loaded before the program goes into multi-threaded mode. What we can do is both, do the reinit at a later point in time if the environment variable happens to be set at that point in time. then you are risking the thread-safety issues only when there is no other way. Or just do it lazily on demand, and work through the libcurl or whatever the unsafety issue is. Greetings, Dirk
[PATCH] libelf: Also copy/convert partial datastructures in xlate functions
The generated xlate functions can only convert full datastructures,
dropping any trailing partial data on the floor. That means some of
the data might be undefined. Just copy over the trailing bytes as
is. That data isn't really usable. But at least it is defined data.
https://sourceware.org/bugzilla/show_bug.cgi?id=29000
Signed-off-by: Mark Wielaard
---
libelf/ChangeLog| 5 +
libelf/gelf_xlate.c | 10 --
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 7fd6202b..299179cb 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@
+2022-03-29 Mark Wielaard
+
+ * gelf_xlate.c (START): Define and use sz variable.
+ (END): Use sz variable to decide whether to do a memmove.
+
2022-03-24 Mark Wielaard
* elf.h: Update from glibc.
diff --git a/libelf/gelf_xlate.c b/libelf/gelf_xlate.c
index b9e7fd65..6f8c57b7 100644
--- a/libelf/gelf_xlate.c
+++ b/libelf/gelf_xlate.c
@@ -1,5 +1,6 @@
/* Transformation functions for ELF data types.
Copyright (C) 1998,1999,2000,2002,2004,2005,2006,2007,2015 Red Hat, Inc.
+ Copyright (C) 2022 Mark J. Wielaard
This file is part of elfutils.
Written by Ulrich Drepper , 1998.
@@ -138,9 +139,14 @@ union unaligned
int encode __attribute__ ((unused))) \
{ ElfW2(Bits, Name) *tdest = (ElfW2(Bits, Name) *) dest; \
ElfW2(Bits, Name) *tsrc = (ElfW2(Bits, Name) *) src; \
+size_t sz = sizeof (ElfW2(Bits, Name)); \
size_t n;\
-for (n = len / sizeof (ElfW2(Bits, Name)); n > 0; ++tdest, ++tsrc, --n) {
-#define END(Bits, Name) } }
+for (n = len / sz; n > 0; ++tdest, ++tsrc, --n) {
+#define END(Bits, Name)
\
+}\
+if (len % sz > 0) /* Cannot convert partial structures, just copy. */ \
+ memmove (dest, src, len % sz); \
+ }
#define TYPE_EXTRA(Code)
#define TYPE_XLATE(Code) Code
#define TYPE_NAME(Type, Name) TYPE_NAME2 (Type, Name)
--
2.30.2
[Bug libelf/29000] Conditional jump or move depends on uninitialised value in elf_compress_gnu
https://sourceware.org/bugzilla/show_bug.cgi?id=29000 Mark Wielaard changed: What|Removed |Added Status|NEW |ASSIGNED Assignee|unassigned at sourceware dot org |mark at klomp dot org --- Comment #3 from Mark Wielaard --- > It can also be triggered on the second example with valgrind eu-readelf -w Thanks, that was very useful. I don't think it really is a bug. But it is use of undefined data. The issue is that if the ELF data structures need to be converted then it only makes sense to convert full datastructures. But just dropping the bad/partial data is not a great idea either. So this proposed patch just copies over the bad/partial data that couldn't be converted. That way it is at least deterministically defined. https://code.wildebeest.org/git/user/mjw/elfutils/commit/?h=fuzz https://sourceware.org/pipermail/elfutils-devel/2022q1/004825.html -- You are receiving this mail because: You are on the CC list for the bug.
