[PATCH] Fix some potential deref-of-null error
strip.c: Pointer `arhdr` created at strip.c:2741 and then dereferenced without
NULL-check.
The same situation for the `arhdr` pointer at the objdump.c:313 and
the `h` pointer at the readelf.c:13545.
Triggers found by static analyzer Svace.
Signed-off-by: Maks Mishin
---
src/objdump.c | 5 +
src/readelf.c | 5 +
src/strip.c | 5 +
3 files changed, 15 insertions(+)
diff --git a/src/objdump.c b/src/objdump.c
index d43c1dd6..8ad8cdb5 100644
--- a/src/objdump.c
+++ b/src/objdump.c
@@ -311,6 +311,11 @@ handle_ar (int fd, Elf *elf, const char *prefix, const
char *fname,
{
/* The the header for this element. */
Elf_Arhdr *arhdr = elf_getarhdr (subelf);
+ if (arhdr == NULL)
+ {
+ printf ("cannot get arhdr: %s\n", elf_errmsg (-1));
+ exit (1);
+ }
/* Skip over the index entries. */
if (strcmp (arhdr->ar_name, "/") != 0
diff --git a/src/readelf.c b/src/readelf.c
index 48035264..96d7877c 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -13543,6 +13543,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)
+ {
+ printf ("cannot get arhdr: %s\n", elf_errmsg (-1));
+ exit (1);
+ }
printf (_("Archive member '%s' contains:\n"), h->ar_name);
diff --git a/src/strip.c b/src/strip.c
index 403e0f6f..44389c9b 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -2739,6 +2739,11 @@ handle_ar (int fd, Elf *elf, const char *prefix, const
char *fname,
{
/* The the header for this element. */
Elf_Arhdr *arhdr = elf_getarhdr (subelf);
+ if (arhdr == NULL)
+ {
+ printf ("cannot get arhdr: %s\n", elf_errmsg (-1));
+ exit (1);
+ }
if (elf_kind (subelf) == ELF_K_ELF)
result |= handle_elf (fd, subelf, new_prefix, arhdr->ar_name, 0, NULL);
--
2.30.2
Re: [PATCH 1/5] aarch64: Create definitions for AARCH64_RA_SIGN_STATE register
On Fri, Jun 14, 2024 at 03:47:15PM +0100, Steve Capper wrote: > From: German Gomez > > This register will be used to indicate whether a return address is > mangled with a PAC or not, in accordance with the DWARF AARCH64 ABI [1]. > > [1] > https://github.com/ARM-software/abi-aa/blob/main/aadwarf64/aadwarf64.rst#41dwarf-register-names OK, so DWARF regno 34 turns from "reserved" into the ra_sign_state. Patch looks good except for... > --- a/backends/aarch64_init.c > +++ b/backends/aarch64_init.c > @@ -55,10 +55,10 @@ aarch64_init (Elf *elf __attribute__ ((unused)), >HOOK (eh, data_marker_symbol); >HOOK (eh, abi_cfi); > > - /* X0-X30 (31 regs) + SP + 1 Reserved + ELR, 30 Reserved regs (34-43) > + /* X0-X30 (31 regs) + SP + 1 Reserved + ELR + RA_SIGN_STATE, 30 Reserved > regs (34-43) > + V0-V31 (32 regs, least significant 64 bits only) > - + ALT_FRAME_RETURN_COLUMN (used when LR isn't used) = 97 DWARF regs. */ > - eh->frame_nregs = 97; > + + ALT_FRAME_RETURN_COLUMN (used when LR isn't used) = 98 DWARF regs. */ > + eh->frame_nregs = 98; >HOOK (eh, set_initial_registers_tid); >HOOK (eh, unwind); That should still be 97. RA_SIGN_STATE is regno 34, which was reserved before, followed by 29 Resered regs (35-43). Cheers, Mark
Re: [PATCH 2/5] libdw, aarch64: Implement DW_CFA_AARCH64_negate_ra_state CFI instruction
Hi,
On Fri, Jun 14, 2024 at 03:47:16PM +0100, Steve Capper wrote:
> From: German Gomez
>
> Implement DW_CFA_AARCH64_negate_ra_state in accordance with the DWARF
> AARCH64 ABI [1].
>
> Followup commits will use the value of this register to remove the PAC
> from return addresses.
Looks correct.
> [1]
> https://github.com/ARM-software/abi-aa/blob/main/aadwarf64/aadwarf64.rst#44call-frame-instructions
>
> Signed-off-by: German Gomez
> Signed-off-by: Steve Capper
> ---
> libdw/cfi.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/libdw/cfi.c b/libdw/cfi.c
> index a7174405..743bfc07 100644
> --- a/libdw/cfi.c
> +++ b/libdw/cfi.c
> @@ -125,6 +125,15 @@ execute_cfi (Dwarf_CFI *cache,
> fs->regs[regno].value = (r_value); \
>} while (0)
>
> + /* The AARCH64 DWARF ABI states that register 34 (ra_sign_state) must
> + be initialized to 0. So do it before executing the CFI. */
> + if (cache->e_machine == EM_AARCH64)
> +{
> + if (unlikely (! enough_registers (DW_AARCH64_RA_SIGN_STATE, &fs,
> &result)))
> + goto out;
> + fs->regs[DW_AARCH64_RA_SIGN_STATE].value = 0;
> +}
> +
>while (program < end)
> {
>uint8_t opcode = *program++;
OK.
> @@ -357,7 +366,10 @@ execute_cfi (Dwarf_CFI *cache,
> {
> /* Toggles the return address state, indicating whether
>the return address is encrypted or not on
> - aarch64. XXX not handled yet. */
> + aarch64. */
> + if (unlikely (! enough_registers (DW_AARCH64_RA_SIGN_STATE, &fs,
> &result)))
> + goto out;
> + fs->regs[DW_AARCH64_RA_SIGN_STATE].value ^= 0x1;
> }
> else
> {
Funny we already had some code here to handle
DW_CFA_AARCH64_negate_ra_state (and how unfortunate it overlaps with
DW_CFA_GNU_window_save, but there is already an em check).
Also looks good.
Thanks,
Mark
Re: [PATCH 1/5] aarch64: Create definitions for AARCH64_RA_SIGN_STATE register
Hi,
Sorry, one more thing.
On Fri, Jun 14, 2024 at 03:47:15PM +0100, Steve Capper wrote:
> index 4be32de5..f6d26044 100644
> --- a/libdw/dwarf.h
> +++ b/libdw/dwarf.h
> @@ -1028,6 +1028,11 @@ enum
> DW_EH_PE_indirect = 0x80
>};
>
> +/* AARCH64 DWARF registers. */
> +enum
> + {
> +DW_AARCH64_RA_SIGN_STATE = 34
> + };
dwarf.h is a public header. I rather not define this DW constant here.
Especially since it doesn't seem to have common name (binutils-gdb
calls this AARCH64_DWARF_RA_SIGN_STATE, but only defines it in a
private header). Since this is really just a private detail for CFI
could we define it in cfi.h (which is a private elfutils header)?
Cheers,
Mark
Re: [PATCH 0/5] Enable PAC support in elfutils
Hi Steve, On Fri, Jun 14, 2024 at 03:47:14PM +0100, Steve Capper wrote: > This series enables Pointer Authentication (PAC) support in elfutils. > > The first four patches were originally posted by German Gomez. I've > rebased to the latest elfutils and added an extra patch that was > required to debug core dumps from PAC enabled applications. > > These patches were tested on Debian Testing and Fedora 40 running on an > Apple M1 MacBook Pro (the CFLAG -mbranch-protection=standard needs to be > supplied to the build). Thanks, I found a setup to test this and it works. Nice. > Without this series applied, the following tests failed: > * run-backtrace-native.sh > * run-backtrace-dwarf.sh > * run-backtrace-native-core.sh > * run-deleted.sh > > I am happy to chop/change bits as necessary. I had some small comments on the first two patches. They look good, just tiny nitpicks. The last three introduce/depend on a new public function dwfl_thread_state_aarch64_pauth. I rather not have such a public architecture specific function. Could we instead try to reuse dwfl_thread_state_registers? We could say that negative regnums are special architecture specific settings? There is already some precedent for that in the the thread_state_registers_cb function given to ebl_set_initial_registers_tid, which call with -1 to set the PC value. We could use -2 to indicate it is an arch specific setting. That is also slightly ugly. But given we can hide most of it in architecture specific/private code better than special case public architecture functions. Cheers, Mark
Re: [PATCH 01/10 v3] libelf: Fix deadlock in __libelf_readall
Hi Mark, On Thu, Aug 15, 2024 at 12:13 PM Mark Wielaard wrote: > > On Fri, 2024-08-02 at 19:38 -0400, Aaron Merey wrote: > > From: Heather McIntyre > > > > Apply locking during __libelf_readall. > > > > Signed-off-by: Heather S. McIntyre > > Signed-off-by: Aaron Merey > > Signed-off-by: Mark Wielaard > > > > --- > > v3 changes: > > > > Update comments and change order or child lock aquisition. > > This looks OK, it addresses all my comments from v2. Thanks, pushed as commit f3de289b508. Aaron
Re: [PATCH 02/10 v3] libelf: Fix deadlock in elf_cntl
Hi Mark, On Thu, Aug 15, 2024 at 12:16 PM Mark Wielaard wrote: > > On Fri, 2024-08-02 at 19:38 -0400, Aaron Merey wrote: > > From: Heather McIntyre > > > > * libelf/elf_cntl.c (elf_cntl): Move rwlock_wrlock, rwlock_unlock, > > inside case switch statements. Remove unnecessary early return. > > > > Signed-off-by: Heather S. McIntyre > > Signed-off-by: Aaron Merey > > Signed-off-by: Mark Wielaard > > > > --- > > v3 changes: > > Remove unnecessary early return that would require locking to > > check condition. > > The code looks good and clean now. Thanks, pushed as commit 97b72c00603. Aaron
Re: [PATCH 03/10 v3] lib: Add eu_tsearch, eu_tfind, eu_tdelete and eu_tdestroy
Hi Mark, On Thu, Aug 15, 2024 at 6:13 PM Mark Wielaard wrote: > > On Fri, Aug 02, 2024 at 07:38:02PM -0400, Aaron Merey wrote: > > From: Heather McIntyre > > > > Add struct search_tree to hold tree root and lock. Add new eu_t* > > functions for ensuring synchronized tree access. > > > > Replace tsearch, tfind, etc with eu_t* equivalents. > [...] > > Looks good to go to me. Thanks, pushed as commit d6443d1a4d. Aaron
Re: [PATCH 04/10 v3] libdw: Add rwlocks for Dwarf and Dwarf_CU
Hi Mark, On Fri, Aug 16, 2024 at 9:38 AM Mark Wielaard wrote: > > Hi Aaron, > > On Fri, 2024-08-02 at 19:38 -0400, Aaron Merey wrote: > > From: Heather McIntyre > > > > Add dwarf_lock for Dwarf as well as abbrev_lock for Dwarf_CU. > > > > * libdw/dwarf_begin_elf.c (dwarf_begin_elf): Init dwarf_lock. > > * libdw/dwarf_end.c (cu_free): Free abbrev_lock. > > (dwarf_end): Free dwarf_lock. > > * libdw/libdwP.h (struct Dwarf): Define dwarf_lock. > > (struct Dwarf_CU): Define abbrev_lock. > > * libdw/libdw_findcu.c (__libdw_intern_next_unit): Init > > abbrev_lock. > > > > Signed-off-by: Heather S. McIntyre > > Signed-off-by: Aaron Merey > > > > --- > > v3 changes: > > New patch added in v3 that contains rwlock changes previously in patch 3/9 > > v2. > > The patch itself looks correct. But I think I would have introduced the > locks separately and/or in the first patch that used them (dwarf_getalt > for the dwarf_lock, dwarf_formref_die and __libdw_dieabbrev for > abbrev_lock). > > It might also be a good idea to explicitly document which structure > fields are "protected" by the separate locks to make reasoning about > them a little easier. Done. The dwarf_lock changes were squashed into commit 7c4fcff44ae and the abbrev_lock changes were squashed into commit 28b74a1bf73. Aaron
Re: [PATCH 05/10 v3] libdw: make dwarf_getalt and dwarf_setalt thread-safe
Hi Mark, On Fri, Aug 16, 2024 at 9:45 AM Mark Wielaard wrote: > > On Fri, 2024-08-02 at 19:38 -0400, Aaron Merey wrote: > > From: Heather McIntyre > > > > * libdw/dwarf_getalt.c (dwarf_getalt): Add locking. > > * libdw/dwarf_setalt.c (dwarf_setalt): Ditto. > > > > Signed-off-by: Heather S. McIntyre > > Signed-off-by: Aaron Merey > > Signed-off-by: Mark Wielaard > > > > --- > > v3 changes: > > > > Check for NULL argument before acquiring lock. Assign result before > > releasing lock. Include dwarf_setalt.c changes that were previously > > included in patch 5/9 v2. > > Cleanups look good. This patch looks good to go. Thanks, pushed as commit 7c4fcff44ae. Aaron
Re: [PATCH 06/10 v3] libdwP.h: Add locking to dwarf_formref_die and __libdw_dieabbrev
Hi Mark,
On Fri, Aug 16, 2024 at 5:51 PM Mark Wielaard wrote:
>
> On Fri, Aug 02, 2024 at 07:38:05PM -0400, Aaron Merey wrote:
> > From: Heather McIntyre
> >
> > * libdw/dwarf_formref_die.c (dwarf_formref_die): Add locking
> > around call to __libdw_intern_next_unit.
> > * libdw/libdwP.h (__libdw_dieabbrev): Add locking.
> >
> > Signed-off-by: Heather S. McIntyre
> > Signed-off-by: Aaron Merey
> > Signed-off-by: Mark Wielaard
> >
> > v3 changes:
> > Move dwarf_setalt.c changes to patch 5/9 v3.
> > Set __libdw_dieabbrev result before releasing lock.
> > ---
> > libdw/dwarf_formref_die.c | 3 +++
> > libdw/libdwP.h| 25 +
> > 2 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/libdw/dwarf_formref_die.c b/libdw/dwarf_formref_die.c
> > index 48ba8194..69d1bf1c 100644
> > --- a/libdw/dwarf_formref_die.c
> > +++ b/libdw/dwarf_formref_die.c
> > @@ -92,7 +92,10 @@ dwarf_formref_die (Dwarf_Attribute *attr, Dwarf_Die
> > *result)
> > bool scan_debug_types = false;
> > do
> > {
> > + rwlock_wrlock(attr->cu->dbg->dwarf_lock);
> > cu = __libdw_intern_next_unit (attr->cu->dbg, scan_debug_types);
> > + rwlock_unlock(attr->cu->dbg->dwarf_lock);
> > +
> > if (cu == NULL)
> > {
> > if (scan_debug_types == false)
>
> Aha, sorry, so there is extra locking around __libdw_intern_next_unit
> here. Shouldn't this be combined with the next patch?
I moved these dwarf_formref_die.c into the patch "libdw: Make
libdw_find_split_unit thread-safe", which was pushed as commit
2630bce747. The rest of the changes were pushed as commit
28b74a1bf73.
Aaron
Re: [PATCH 07/10 v3] libdw: Make libdw_find_split_unit thread-safe
Hi Mark, On Fri, Aug 16, 2024 at 5:14 PM Mark Wielaard wrote: > > On Fri, Aug 02, 2024 at 07:38:06PM -0400, Aaron Merey wrote: > > From: Heather McIntyre > > > > * libdw/libdwP.h (struct Dwarf_CU): Add split_lock. > > * libdw/libdw_find_split_unit.c (__libdw_find_split_unit): > > Add locking. > > > > Signed-off-by: Heather S. McIntyre > > Signed-off-by: Aaron Merey > > Signed-off-by: Mark Wielaard > > > > --- > > v3 changes: > > Move Dwarf split_lock code from patch 3/9 v2 to this patch. > > Assign return value before releasing lock. > > Looks good this way. Thanks, pushed as commit 2630bce7476. Aaron
Re: [PATCH 08/10 v3] libdw: Make libdw_findcu thread-safe
Hi Mark, On Fri, Aug 16, 2024 at 5:38 PM Mark Wielaard wrote: > > Hi, > > On Fri, Aug 02, 2024 at 07:38:07PM -0400, Aaron Merey wrote: > > From: Heather McIntyre > > > > * libdw/libdw_findcu.c (__libdw_findcu): Add locking. > > > > Signed-off-by: Heather S. McIntyre > > Signed-off-by: Aaron Merey > > Signed-off-by: Mark Wielaard > > > > --- > > v3 changes: > > Fix indentation and move rwlock_init calls to other patches in this > > series. > > So this is basically a lock around __libdw_intern_next_unit. But there > is also a call to __libdw_intern_next_unit from dwarf_formref_die. So > there should also be a lock there? The locking around __libdw_intern_next_unit can be found in commit 2630bce74 (libdw: Make libdw_find_split_unit thread-safe). This patch was pushed as commit a0a2996d15. Aaron
☠ Buildbot (Sourceware): elfutils-snapshots-coverage - failed test (failure) (main)
A new failure has been detected on builder elfutils-snapshots-coverage while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#/builders/250/builds/156 Build state: failed test (failure) Revision: a0a2996d15529673bf43269fe7fcafa2e4a0187b Worker: snapshots Build Reason: (unknown) Blamelist: Heather McIntyre Steps: - 0: worker_preparation ( success ) - 1: git checkout ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/1/logs/stdio - 2: autoreconf ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/2/logs/stdio - 3: configure ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/3/logs/stdio - config.log: https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/3/logs/config_log - 4: make ( warnings ) Logs: - stdio: https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/4/logs/stdio - warnings (3): https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/4/logs/warnings__3_ - 5: make check ( failure ) Logs: - stdio: https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/5/logs/stdio - test-suite.log: https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/5/logs/test-suite_log - 6: make coverage ( warnings ) Logs: - stdio: https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/6/logs/stdio - warnings (12): https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/6/logs/warnings__12_ - 7: Wait snapshots output ready ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/7/logs/stdio - 8: create output ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/8/logs/stdio - 9: create publish file ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#/builders/250/builds/156/steps/9/logs/stdio
☝ Buildbot (Sourceware): elfutils - worker not available (main)
A retry build has been detected on builder elfutils-opensuseleap-x86_64 while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#/builders/90/builds/334 Build state: worker not available Revision: (unknown) Worker: bb1-1 Build Reason: (unknown) Blamelist: Heather McIntyre Steps: - 0: worker_preparation ( exception )
☝ Buildbot (Sourceware): elfutils - worker not available (main)
A retry build has been detected on builder elfutils-opensuseleap-x86_64 while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#/builders/90/builds/333 Build state: worker not available Revision: (unknown) Worker: bbo1-2 Build Reason: (unknown) Blamelist: Heather McIntyre Steps: - 0: worker_preparation ( exception )
☝ Buildbot (Sourceware): elfutils - worker not available (main)
A retry build has been detected on builder elfutils-opensuseleap-x86_64 while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#/builders/90/builds/332 Build state: worker not available Revision: (unknown) Worker: bbo1-1 Build Reason: (unknown) Blamelist: Heather McIntyre Steps: - 0: worker_preparation ( exception ) A cancelled build has been detected on builder elfutils-opensuseleap-x86_64 while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#/builders/90/builds/336 Build state: worker cancelled Revision: (unknown) Worker: bb2-1 Build Reason: (unknown) Blamelist: Aaron Merey , Aleksei Vetrov , Alfred Wingate , Andreas Schwab , Di Chen , Frank Ch. Eigler , Frederik “Freso” S. Olesen , Heather McIntyre , Housam Alamour , Jose Quaresma , Khem Raj , Kuan-Ying Lee , Luca Boccassi , Luke Diamand , Maks Mishin , Mark Wielaard , Matheus Tavares Bernardino , Michal Sekletar , Norbert Lange , Omar Sandoval , Paul Pluzhnikov , Sergei Trofimovich , Xi Ruoyao , Ying Huang Steps: - 0: worker_preparation ( cancelled )
