Re: [PATCH] src: fix DEREF_OF_NULL.RET.STAT in readelf.c in
Hi Anton, On Thu, Feb 13, 2025 at 07:33:12PM +0300, Anton Moryakov wrote: > Static analyzer reported: > Return value of a function 'elf_strptr' is dereferenced at readelf.c:7171 > without checking for NULL, but it is usually checked for this function > (71/74). > > Corrections explained: > - Added a NULL check for the scnname variable, which contains the result of > the elf_strptr call. > - The check is placed before the first use of scnname to prevent dereferencing > a NULL pointer. Yes, I see why a static analyzer might assume that. But if you look at the caller of print_debug_frame_section, the function print_debug, then you'll see that it already does this check (at line 12140): const char *name = elf_strptr (ebl->elf, shstrndx, shdr->sh_name); if (name == NULL) continue; And at the start of print_debug_frame_section we do have: /* We know this call will succeed since it did in the caller. */ (void) elf_getshdrstrndx (ebl->elf, &shstrndx); const char *scnname = elf_strptr (ebl->elf, shstrndx, shdr->sh_name); So I don't think scnname can be NULL. But this code is different from any other print_debug_* code. All other code uses section_name (Ebl *ebl, GElf_Shdr *shdr) to get the section name. So what we could do to make the static analyzer happy is simply do the same here. I pushed the attached. Cheers, Mark >From dfa7b2c23ddabcba2a4972fa67d3c670ae31f1ee Mon Sep 17 00:00:00 2001 From: Mark Wielaard Date: Thu, 27 Feb 2025 21:22:49 +0100 Subject: [PATCH] readelf: Use section_name instead of elf_strptr in print_debug_frame_section All other print_debug_* functions use section_name (ebl, shdr) to get the current section name. Be consistent and use the same method in print_debug_frame_section to make static analyzers happy who might think elf_strptr can return NULL in this case. * src/readelf.c (print_debug_frame_section): Use section_name instead of elf_strptr to get the section name. Signed-off-by: Mark Wielaard --- src/readelf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/readelf.c b/src/readelf.c index 61d5b71a7913..c9aebd5b3e5f 100644 --- a/src/readelf.c +++ b/src/readelf.c @@ -7170,7 +7170,7 @@ print_debug_frame_section (Dwfl_Module *dwflmod, Ebl *ebl, GElf_Ehdr *ehdr, size_t shstrndx; /* We know this call will succeed since it did in the caller. */ (void) elf_getshdrstrndx (ebl->elf, &shstrndx); - const char *scnname = elf_strptr (ebl->elf, shstrndx, shdr->sh_name); + const char *scnname = section_name (ebl, shdr); /* Needed if we find PC-relative addresses. */ GElf_Addr bias; -- 2.48.1
Re: [PATCH] scr: fix DEREF_OF_NULL.RET.STAT in ar.c
Hi Anton,
The subject isn't super helpful unless you know the specific
terminology of the statuc analyzer you are using. It would be better to
say something like:
ar: check whether elf_getarhdr returns NULL
On Thu, 2025-02-13 at 18:00 +0300, Anton Moryakov wrote:
> Report of the static analyzer:
> 1. DEREF_OF_NULL.RET Pointer, returned from function 'elf_getarhdr' at
> ar.c:498, may be NULL and is dereferenced at ar.c:500.
> 2. DEREF_OF_NULL.RET Pointer, returned from function 'elf_getarhdr' at
> ar.c:940, may be NULL and is dereferenced at ar.c:943
> 3. DEREF_OF_NULL.RET Pointer, returned from function 'elf_getarhdr' at
> ar.c:1147, may be NULL and is dereferenced at ar.c:1150.
> 4. DEREF_OF_NULL.RET.STAT Return value of a function 'elf_rawfile' is
> dereferenced at ar.c:857 without checking for NULL, but it is usually checked
> for this function (4/5)
>
> Corrections explained:
> Added check if (arhdr == NULL) goto next;
>
> Triggers found by static analyzer Svace.
>
> Signed-off-by: Anton Moryakov
>
> ---
> src/ar.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/src/ar.c b/src/ar.c
> index 9ace28b9..4b90115d 100644
> --- a/src/ar.c
> +++ b/src/ar.c
> @@ -498,6 +498,9 @@ do_oper_extract (int oper, const char *arfname, char
> **argv, int argc,
>while ((subelf = elf_begin (fd, cmd, elf)) != NULL)
> {
>Elf_Arhdr *arhdr = elf_getarhdr (subelf);
> +
> + if (arhdr == NULL)
> + goto next;
>
OK, but the identation is completely wrong. There is extra whitespace
on the first line, just remove the line. The if line should be indented
6 spaces, the goto line should be indented one tab.
>if (strcmp (arhdr->ar_name, "/") == 0)
> {
> @@ -860,6 +863,9 @@ write_member (struct armem *memb, off_t *startp, off_t
> *lenp, Elf *elf,
> {
>/* In case of a long file name we assume the archive header
>changed and we write it here. */
> +
> + if (arhdr == NULL)
> + goto next;
>memcpy (&arhdr, elf_rawfile (elf, NULL) + *startp, sizeof (arhdr));
This doesn't make sense to me, does it even compile?
arhdr is a struct ar_hdr not a pointer to it.
>snprintf (tmpbuf, sizeof (tmpbuf), "/%-*ld",
> @@ -943,6 +949,9 @@ do_oper_delete (const char *arfname, char **argv, int
> argc,
>while ((subelf = elf_begin (fd, cmd, elf)) != NULL)
> {
>Elf_Arhdr *arhdr = elf_getarhdr (subelf);
> +
> + if (arhdr == NULL)
> + goto next;
This does make sense, but indentation needs to be fixed like above.
>/* Ignore the symbol table and the long file name table here. */
>if (strcmp (arhdr->ar_name, "/") == 0
> @@ -1152,6 +1161,9 @@ do_oper_insert (int oper, const char *arfname, char
> **argv, int argc,
>while ((subelf = elf_begin (fd, cmd, elf)) != NULL)
> {
>Elf_Arhdr *arhdr = elf_getarhdr (subelf);
> +
> + if (arhdr == NULL)
> + goto next;
Likewise.
Thanks,
Mark
Re: [PATCH] src: fix DEREF_OF_NULL.RET in readelf.c
Hi Anton,
On Thu, 2025-02-13 at 18:57 +0300, Anton Moryakov wrote:
> Report of the static analyzer:
> DEREF_OF_NULL.RET Pointer, returned from function 'elf_getarhdr' at
> readelf.c:13551,
> may be NULL and is dereferenced at readelf.c:13553.
>
> Corrections explained:
> - Added a NULL check for the pointer returned by `elf_getarhdr`.
> - If the pointer is NULL, release resources with `elf_end` and skip
> the current iteration using `continue`.
>
> Triggers found by static analyzer Svace.
>
> Signed-off-by: Anton Moryakov
> ---
> src/readelf.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/readelf.c b/src/readelf.c
> index 6526db07..4c14fc21 100644
> --- a/src/readelf.c
> +++ b/src/readelf.c
> @@ -13549,7 +13549,11 @@ dump_archive_index (Elf *elf, const char *fname)
> as_off, fname, elf_errmsg (-1));
>
> const Elf_Arhdr *h = elf_getarhdr (subelf);
> -
> + if (h == NULL)
> + {
> + elf_end(subelf);
> + continue;
> + }
> printf (_("Archive member '%s' contains:\n"), h->ar_name);
>
> elf_end (subelf);
Again subject isn't super helpful and indentation is incorrect.
Also it is easier to switch the check around to:
+ if (h != NULL)
+ printf (_("Archive member '%s' contains:\n"), h->ar_name);
I made those changes and checked this in.
Thanks,
Mark
Re: [PATCH 2/9 v3] libdw: Add locking to dwarf_getsrcfiles, dwarf_getsrclines, dwarf_macro_getsrcfiles
Hi Aaron,
On Wed, Feb 19, 2025 at 11:36:37PM -0500, Aaron Merey wrote:
> * libdw/dwarf_getsrcfiles.c (dwarf_getsrcfiles): Use dwarf_lock.
> * libdw/dwarf_getsrclines.c (dwarf_getsrclines): Ditto.
> * libdw/dwarf_macro_getsrclines.c (dwarf_macro_getsrclines):
> Ditto.
>
> Signed-off-by: Aaron Merey
>
> ---
> v3 changes:
> This patch replaces v2 02/10 and v2 03/10.
>
> The locking to dwarf_filesrc in 02/10 has been removed. The removal of
> dwarf_filesrc locking triggered an error from helgrind that is now fixed
> by the dwarf_macro_getsrclines locking added in this patch.
That makes sense.
> On Wed, Feb 12, 2025 at 8:17 AM Mark Wielaard wrote:
> >
> > Hi Aaron,
> >
> > On Tue, 2025-02-04 at 16:50 -0500, Aaron Merey wrote:
> > > * libdw/dwarf_getsrcfiles.c (dwarf_getsrcfiles): Use dwarf_lock.
> > > * libdw/dwarf_getsrclines.c (dwarf_getsrclines): Use dwarf_lock.
> > >
> > > Signed-off-by: Aaron Merey
> > > ---
> > > v2 changes: Combined from v1 patches 04/15 and 05/15.
> > >
> > > libdw/dwarf_getsrcfiles.c | 11 +++
> > > libdw/dwarf_getsrclines.c | 25 +++--
> > > 2 files changed, 26 insertions(+), 10 deletions(-)
> >
> > OK, so here dwarf_lock is used to guard access to the Dwarf_CU fields
> > lines and files. Might it make sense to turn these into a Dwarf_CU
> > specific lock?
>
> We could add another Dwarf_CU lock for this purpose. However we'll still
> need to use the dwarf_lock when lines or files haven't been read. For now
> I'd prefer to keep the number of locks as low as possible and focus on
> performance improvements once we've achieved thread safety.
>
> OTOH it shouldn't be too much work to add this cu lock so please let me
> know if you'd still like to see it added in this patch.
We should make dwarf_getsrcfiles and dwarf_getsrclines independent so
callers that onle need the file table don't need to "pay" for parsing
the whole line table. (I thought we had a bug for that, but cannot
find it now. If you also cannot find it, please let me know and I'll
file a new one.)
When we do that, we can reevaluate the locking situation.
I do think there should be lock per CU since now (especially since we
are using mutex) if you try to get the the file and/or line table for
one CU you block all threads working on other CUs in the Dwarf
Debug. So you are serializing the file/line table parsing.
> diff --git a/libdw/dwarf_getsrcfiles.c b/libdw/dwarf_getsrcfiles.c
> index 24e4b7d2..3029ce69 100644
> --- a/libdw/dwarf_getsrcfiles.c
> +++ b/libdw/dwarf_getsrcfiles.c
> @@ -47,9 +47,10 @@ dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files,
> size_t *nfiles)
> }
>
>int res = -1;
> + struct Dwarf_CU *const cu = cudie->cu;
> + mutex_lock (cudie->cu->dbg->dwarf_lock);
>
>/* Get the information if it is not already known. */
> - struct Dwarf_CU *const cu = cudie->cu;
>if (cu->files == NULL)
> {
>/* For split units there might be a simple file table (without lines).
> @@ -96,7 +97,10 @@ dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files,
> size_t *nfiles)
> Dwarf_Off debug_line_offset;
> if (__libdw_formptr (stmt_list, IDX_debug_line, DWARF_E_NO_DEBUG_LINE,
> NULL, &debug_line_offset) == NULL)
> - return -1;
> + {
> + mutex_unlock (cudie->cu->dbg->dwarf_lock);
> + return -1;
> + }
>
> res = __libdw_getsrcfiles (cu->dbg, debug_line_offset,
>__libdw_getcompdir (cudie),
> @@ -115,8 +119,7 @@ dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files,
> size_t *nfiles)
> *nfiles = cu->files->nfiles;
> }
>
> - // XXX Eventually: unlocking here.
> -
> + mutex_unlock (cudie->cu->dbg->dwarf_lock);
>return res;
> }
> INTDEF (dwarf_getsrcfiles)
This looks correct. Minor nit about if { } indenting, which should be:
if (...)
{
x;
}
> diff --git a/libdw/dwarf_getsrclines.c b/libdw/dwarf_getsrclines.c
> index da78db67..c2e686b1 100644
> --- a/libdw/dwarf_getsrclines.c
> +++ b/libdw/dwarf_getsrclines.c
> @@ -1442,8 +1442,10 @@ dwarf_getsrclines (Dwarf_Die *cudie, Dwarf_Lines
> **lines, size_t *nlines)
>return -1;
> }
>
> - /* Get the information if it is not already known. */
>struct Dwarf_CU *const cu = cudie->cu;
> + mutex_lock (cu->dbg->dwarf_lock);
> +
> + /* Get the information if it is not already known. */
>if (cu->lines == NULL)
> {
>/* For split units always pick the lines from the skeleton. */
> @@ -1464,10 +1466,13 @@ dwarf_getsrclines (Dwarf_Die *cudie, Dwarf_Lines
> **lines, size_t *nlines)
> *lines = cu->lines;
> *nlines = cu->lines->nlines;
> }
> +
> + mutex_unlock (cu->dbg->dwarf_lock);
> return res;
> }
>
> __libdw_seterrno (DWARF_E_NO_DEBUG_LINE);
> + mutex_unlock (cu->dbg->dwa
Re: [PATCH 3/9 v3] libdwP.h: Add locking to str_offsets_base_off
Hi Aaron,
On Wed, Feb 19, 2025 at 11:36:38PM -0500, Aaron Merey wrote:
> * libdw/dwarf_end.c (cu_free): Free str_off_base_lock.
> * libdw/libdwP.h (struct Dwarf_CU): Add str_off_base_lock member.
> (str_offsets_base_off): Add locking.
> * libdw/libdw_findcu.c (__libdw_intern_next_unit): Initialize
> str_off_base_lock.
>
> Signed-off-by: Aaron Merey
>
> ---
> v3 changes:
> This patch replaces v2 04/10. 04/10 added a lock to dwarf_offdie that
> was unnecessary but happened to prevent the reporting of a race condition
> in str_offsets_base_off. This patch addresses the source of the race
> condition.
O wow, yeah, that is "calculated" lazily. Nice you are using a cu
local mutext lock in this case. I think a rwlock might be more
appropriate here (but will not insist on it). But here you would
mostly just "read" the offset (and all those read locks can run in
parallel), while the write lock (which locks out every other thread)
only has to be taken once when setting up the offset.
The only worry I have now is that we probably also need this for
base_address, addr_base, ranges_base and locs_base. It could a shared
"base" lock.
> libdw/dwarf_end.c| 1 +
> libdw/libdwP.h | 20 ++--
> libdw/libdw_findcu.c | 1 +
> 3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
> index 92389c07..d8830823 100644
> --- a/libdw/dwarf_end.c
> +++ b/libdw/dwarf_end.c
> @@ -70,6 +70,7 @@ cu_free (void *arg)
>Dwarf_Abbrev_Hash_free (&p->abbrev_hash);
>rwlock_fini (p->abbrev_lock);
>rwlock_fini (p->split_lock);
> + mutex_fini (p->str_off_base_lock);
>
>/* Free split dwarf one way (from skeleton to split). */
>if (p->unit_type == DW_UT_skeleton
OK, inited in libdw_findcu below.
> diff --git a/libdw/libdwP.h b/libdw/libdwP.h
> index d121914d..27e968ae 100644
> --- a/libdw/libdwP.h
> +++ b/libdw/libdwP.h
> @@ -461,6 +461,10 @@ struct Dwarf_CU
> Covers __libdw_find_split_unit. */
>rwlock_define(, split_lock);
>
> + /* Synchronize access to the str_off_base of this Dwarf_CU.
> + Covers __libdw_str_offsets_base_off. */
> + mutex_define(, str_off_base_lock);
> +
>/* Memory boundaries of this CU. */
>void *startp;
>void *endp;
OK. CU lock nice. So maybe share this between all
> @@ -1202,6 +1206,7 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
>Dwarf_Off off = 0;
>if (cu != NULL)
> {
> + mutex_lock (cu->str_off_base_lock);
>if (cu->str_off_base == (Dwarf_Off) -1)
> {
> Dwarf_Off dwp_offset;
> @@ -1216,6 +1221,7 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
> if (dwarf_formudata (&attr, &base) == 0)
> {
> cu->str_off_base = off + base;
> + mutex_unlock (cu->str_off_base_lock);
> return cu->str_off_base;
> }
> }
> @@ -1223,6 +1229,7 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
> if (cu->version < 5)
> {
> cu->str_off_base = off;
> + mutex_unlock (cu->str_off_base_lock);
> return cu->str_off_base;
> }
>
> @@ -1230,7 +1237,12 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
> dbg = cu->dbg;
> }
>else
> - return cu->str_off_base;
> + {
> + mutex_unlock (cu->str_off_base_lock);
> + return cu->str_off_base;
> + }
> +
> + mutex_unlock (cu->str_off_base_lock);
> }
OK, so if we get here we don't have a lock anymore or cu == NULL. If
cu != NULL then off might or might not be set to dwp_offset and
cu->version >= 5 I would maybe not unlock here (just add a comment
that says the lock is held now if cu != NULL). See below.
>/* No str_offsets_base attribute, we have to assume "zero".
> @@ -1280,7 +1292,11 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
>
> no_header:
>if (cu != NULL)
> -cu->str_off_base = off;
> +{
> + mutex_lock (cu->str_off_base_lock);
> + cu->str_off_base = off;
> + mutex_unlock (cu->str_off_base_lock);
> +}
>
>return off;
So here we reaquire the lock, set the str_off_base and drop the lock
again. So I think this is fine, all threads competing to calculate off
and setting str_off_base should calculate the same.
But I think it is easier to reason about this code (and would maybe be
slightly more efficient if there is a lot of contention here) to
simply not drop the lock earlier and just make this:
if (cu != NULL)
{
cu->str_off_base = off;
mutex_unlock (cu->str_off_base_lock);
}
return off;
> }
> diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c
> index 613f61c8..1e96110b 100644
> --- a/libdw/libdw_findcu.c
> +++ b/libdw/libdw_findcu.c
> @@ -179,6 +179,7 @@ __libdw_intern_next_unit (Dwarf *dbg, bool debug_types)
>eu_search_tree_init (&newp->locs_tree);
>rwlock_init (newp->
Re: [PATCH 2/9 v3] libdw: Add locking to dwarf_getsrcfiles, dwarf_getsrclines, dwarf_macro_getsrcfiles
Hi - > We should make dwarf_getsrcfiles and dwarf_getsrclines independent so > callers that onle need the file table don't need to "pay" for parsing > the whole line table. (I thought we had a bug for that, but cannot > find it now. If you also cannot find it, please let me know and I'll > file a new one.) I believe you're referring to PR27405, which amerey himself implemented in commit d4b0848be. > [...] So you are serializing the file/line table parsing. (On the other hand, at this stage of concurrency management in elfutils, it may make perfect sense to err on the side of *correctness* now, and only complicating locking later when serious contention hits to performance are indicated.) - FChE
Re: [patch] PR31862: debuginfod client should cache received x-debuginfod-* headers
Hi Frank,
On Fri, Feb 21, 2025 at 3:07 PM Frank Ch. Eigler wrote:
>
>
> commit 082c0a94eed6706753e8019ce348be095deb72f9 (HEAD -> main)
> Author: Frank Ch. Eigler
> Date: Fri Feb 21 14:33:49 2025 -0500
>
> PR31862: debuginfod: client to cache x-debuginfod-* headers
>
> This feature allows the extra http headers from debuginfod to be saved
> into the client cache, and also thus replayed to clients. This way
> they can perform IMA verification again, if they like, or a federating
> caching intermediate debuginfod server can replay the headers it
> received previously from upstream to future downstream. The headers
> are placed adjacent to the payload files .cache/debuginfod/BUILDID/PAYLOAD
> as .cache/debuginfod/BUILDID/hdr-PAYLOAD. They are aged the same
> atime-based way.
>
> Testing via an extension of a previous small test, and hand-running both
> client & server code under valgrind. No memcheck errors reported.
On my machine memcheck reports leaks due to target_cachehdr_path
missing a free. Leaks are only reported during
run-debuginfod-ima-verification.sh and run-srcfiles-self.sh. This
is surprising since I would have assumed that many more debuginfod
tests end up leaking memory for this variable.
>
> Signed-off-by: Frank Ch. Eigler
>
> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> index d89beae93ea1..3ea234abb1a8 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -569,7 +569,7 @@ debuginfod_clean_cache(debuginfod_client *c,
> return -errno;
>
>regex_t re;
> - const char * pattern =
> ".*/(metadata.*|[a-f0-9]+(/debuginfo|/executable|/source.*|))$"; /* include
> dirs */
> + const char * pattern =
> ".*/(metadata.*|[a-f0-9]+(/hdr.*|/debuginfo|/executable|/source.*|))$"; /*
> include dirs */
>/* NB: also matches .../section/ subdirs, so extracted section files also
> get cleaned. */
>if (regcomp (&re, pattern, REG_EXTENDED | REG_NOSUB) != 0)
> return -ENOMEM;
> @@ -1777,7 +1777,9 @@ debuginfod_query_server_by_buildid (debuginfod_client
> *c,
>char *cache_miss_path = NULL;
>char *target_cache_dir = NULL;
>char *target_cache_path = NULL;
> + char *target_cachehdr_path = NULL;
>char *target_cache_tmppath = NULL;
> + char *target_cachehdr_tmppath = NULL;
>char suffix[NAME_MAX];
>char build_id_bytes[MAX_BUILD_ID_BYTES * 2 + 1];
>int vfd = c->verbose_fd;
> @@ -1912,6 +1914,8 @@ debuginfod_query_server_by_buildid (debuginfod_client
> *c,
> target_cache_path: $HOME/.cache/0123abcd/debuginfo
> target_cache_path: $HOME/.cache/0123abcd/executable-.debug_info
> target_cache_path: $HOME/.cache/0123abcd/source-HASH-#PATH#TO#SOURCE
> + target_cachehdr_path: $HOME/.cache/0123abcd/hdr-debuginfo
> + target_cachehdr_path: $HOME/.cache/0123abcd/hdr-executable...
>*/
>
>cache_path = make_cache_path();
> @@ -1923,11 +1927,15 @@ debuginfod_query_server_by_buildid (debuginfod_client
> *c,
>xalloc_str (target_cache_dir, "%s/%s", cache_path, build_id_bytes);
>(void) mkdir (target_cache_dir, 0700); // failures with this mkdir would
> be caught later too
>
> - if (suffix[0] != '\0') /* section, source queries */
> + if (suffix[0] != '\0') { /* section, source queries */
> xalloc_str (target_cache_path, "%s/%s-%s", target_cache_dir, type,
> suffix);
> - else
> +xalloc_str (target_cachehdr_path, "%s/hdr-%s-%s", target_cache_dir,
> type, suffix);
> + } else {
> xalloc_str (target_cache_path, "%s/%s", target_cache_dir, type);
> +xalloc_str (target_cachehdr_path, "%s/hdr-%s", target_cache_dir, type);
There is some stray white space at the end of this line and elsewhere.
Also these two xalloc_str are the source of the target_cachehdr_path
leak I mentioned above.
> + }
>xalloc_str (target_cache_tmppath, "%s.XX", target_cache_path);
> + xalloc_str (target_cachehdr_tmppath, "%s.XX", target_cachehdr_path);
>
>/* XXX combine these */
>xalloc_str (interval_path, "%s/%s", cache_path,
> cache_clean_interval_filename);
> @@ -1978,6 +1986,50 @@ debuginfod_query_server_by_buildid (debuginfod_client
> *c,
>/* Success */
>update_atime(fd);
>rc = fd;
> +
> + /* Attempt to transcribe saved headers. */
> + int fdh = open (target_cachehdr_path, O_RDONLY);
> + if (fdh >= 0)
> +{
> + if (fstat (fdh, &st) == 0)
> +if (st.st_size > 0)
> + {
> +c->winning_headers = malloc(st.st_size);
> +if (NULL != c->winning_headers) {
> +
I think '{' was meant to be on this empty line.
> + // Read all teh bytes, thanks posix.
> + // In case of any error, unlink the file to force a
> redownload later.
> + off_t hdr_read = 0;
> + while (hdr_read <
Re: [PATCH] src: fix DEREF_OF_NULL.RET.STAT in readelf.c in
Hi Anton,
On Thu, Feb 13, 2025 at 07:52:00PM +0300, Anton Moryakov wrote:
> Static analyzer reported:
> Return value of a function 'gelf_getehdr' is dereferenced at readelf.c:12443
> without checking for NULL, but it is usually checked for this function
> (53/54).
I can see how a static analyzer thinks this gelf_getehdr call can
fail. But it really cannot in this case. The core Elf was already
checked for ehdr->type == ET_CORE in one of the callers. Or we
wouldn't have gotten here.
What you could do to help the analyzer, and make this code slightly
cleaner, is pass down that ehdr from handle_notes through the various
handle_* functions.
Cheers,
Mark
> Corrections explained:
> - Added a NULL check for the ehdr variable
>
> Triggers found by static analyzer Svace.
>
> Signed-off-by: Anton Moryakov
> ---
> src/readelf.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/src/readelf.c b/src/readelf.c
> index 6526db07..3bdfb391 100644
> --- a/src/readelf.c
> +++ b/src/readelf.c
> @@ -12440,6 +12440,11 @@ handle_core_item (Elf *core, const Ebl_Core_Item
> *item, const void *desc,
>field went into the high half of USEC. */
> GElf_Ehdr ehdr_mem;
> GElf_Ehdr *ehdr = gelf_getehdr (core, &ehdr_mem);
> + if (unlikely(ehdr == NULL))
> + {
> + fprintf(stderr, "Failed to get ELF header\n");
> + return;
> + }
> if (likely (ehdr->e_ident[EI_DATA] == ELFDATA2MSB))
> usec >>= 32;
> else
> --
> 2.30.2
>
Re: [PATCH 1/9 v3] Change type of dwarf_lock from rwlock to mutex
Hi Aaron, On Wed, Feb 19, 2025 at 11:36:36PM -0500, Aaron Merey wrote: > Change type of dwarf_lock to mutex in order to take advantage of > built-in support for recursive locking. > > * lib/locks.h: Add macros for locking, unlocking, initializing > and destroying mutexes. > * libdw/dwarf_begin_elf.c (dwarf_end): Replace rwlock macro with > mutex macro. > * libdw/dwarf_formref_die.c (dwarf_formref_die): Ditto. > * libdw/dwarf_getalt.c (dwarf_getalt): Ditto. > * libdw/dwarf_setalt.c (dwarf_setalt): Ditto. > * libdw/libdwP.h (struct Dwarf): Ditto. > * libdw/libdw_findcu.c (__libdw_findcu): Ditto. > > Signed-off-by: Aaron Merey > > --- > v3 changes: More specific comment for dwarf_lock declaration. Still not a fan of these recursive mutexes, but the changes themselves look correct and the extra comments are helpful. Cheers, Mark
Re: [PATCH] src: fix DEREF_OF_NULL.RET.STAT in unstrip.c
Hi Anton, On Thu, Feb 13, 2025 at 08:19:44PM +0300, Anton Moryakov wrote: > Static analyzer reported: > Return value of a function 'elf_getdata' is dereferenced at unstrip.c:1977 > without checking for NULL, but it is usually checked for this function > (97/101). > > Corrections explained: > - Added a check for NULL for the symstrdata variable before calling > dwelf_strtab_finalize. > - If symstrdata is NULL, the program exits with an error. > > Triggers found by static analyzer Svace. > > Signed-off-by: Anton Moryakov > --- > src/unstrip.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/unstrip.c b/src/unstrip.c > index d70053de..35c04700 100644 > --- a/src/unstrip.c > +++ b/src/unstrip.c > @@ -1974,6 +1974,9 @@ more sections in stripped file than debug file -- > arguments reversed?")); > } > } > > + if (symstrdata == NULL) > + error_exit (0, "Failed to get data from symbol string table"); > + >if (dwelf_strtab_finalize (symstrtab, symstrdata) == NULL) > error_exit (0, "Not enough memory to create symbol table"); If you check this why not at the point where elf_getdata is called (symstrdata is assigned?). And then you should also check the other elf_getdata call at the same time here: symdata = elf_getdata (unstripped_symtab, NULL); symstrdata = elf_getdata (unstripped_strtab, NULL); Thanks, Mark
[PATCH] Take latest of archive and file mtime
attached >From dad01d11ce8390f1c32fa39963d6aeb17897a9c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Llu=C3=ADs=20Batlle=20i=20Rossell?= Date: Thu, 27 Feb 2025 14:16:41 +0100 Subject: [PATCH] Take latest of archive and file mtime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Distributions like Yocto update the RPMs but they set all files inside to a fixed timestamp, so that internal timestamp doesn't tell if files changed. Signed-off-by: Lluís Batlle i Rossell --- debuginfod/debuginfod.cxx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx index 0edd57cb..4a65e57d 100644 --- a/debuginfod/debuginfod.cxx +++ b/debuginfod/debuginfod.cxx @@ -4725,7 +4725,9 @@ archive_classify (const string& rps, string& archive_extension, int64_t archivei .bind(2, fileid) .bind(3, seekable_size) .bind(4, seekable_offset) - .bind(5, seekable_mtime) + // Distros like yocto reset timestamp in archives + // Pick the most recent mtime, archive vs entry + .bind(5, seekable_mtime > mtime ? seekable_mtime : mtime) .step_ok_done(); } else // potential source - sdef record -- 2.47.0
