[RESEND PATCH] libpci: sysfs: Fix segmentation fault by including libgen.h

[RESEND PATCH] libpci: sysfs: Fix segmentation fault by including libgen.h

[Jules Maselbas]

On a musl-based system (Alpine-linux) the basename(3) function is not defined
by including string.h with _GNU_SOURCE defined. However basename(3) could be
defined by including libgen.h.

On musl this is a problem than can lead to a segmentation fault, as I have
experienced. This issue is caused by basename(3) function being implicitly
declared and thus having, implicitly, a return type of int. Which in my case
caused an erroneous sign extension of a pointer leading to a segmentation
fault.

Adding an include for libgen.h sound to me like a proper solution.
Also by doing so the `_GNU_SOURCE` defined is no longer needed.


You can find below details on the issue, on alpine linux (x86_64) running:
    $ lspci -s 00:01.0 -v
    00:01.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Family 17h-19h PCIe Dummy Host Bridge (rev 01)
    Segmentation fault

After debugging, the fault is indirectly caused by this compilation warning:
    sysfs.c: In function 'sysfs_fill_info':
    sysfs.c:457:53: warning: implicit declaration of function 'basename' [-Wimplicit-function-declaration]
      457 |           pci_set_property(d, PCI_FILL_IOMMU_GROUP, basename(group_link));
          |                                                     ^~~~~~~~
    sysfs.c:457:53: warning: passing argument 3 of 'pci_set_property' makes pointer from integer without a cast [-Wint-conversion]
      457 |           pci_set_property(d, PCI_FILL_IOMMU_GROUP, basename(group_link));
          |                                                     ^~~~~~~~~~~~~~~~~~~~ int

Here is the relevant assembly, dump from gdb:
    0x7ffff7f4f072  call   *0x5db8(%rip)  # call to basename
    0x7ffff7f4f078  mov    %rbx,%rdi
    0x7ffff7f4f07b  mov    $0x4000,%esi   # PCI_FILL_IOMM_GROUP
    0x7ffff7f4f080  movslq %eax,%rdx      # return value of basename is signed extended from 32bit (eax) to 64bit (rdx)
    0x7ffff7f4f083  call   0x7ffff7f4831b # call to pci_set_property

And how the address becomes invalid:
    (gdb) x/s 0x7ffff7ffed20 # argument of basename
    0x7ffff7ffed20: "/sys/kernel/iommu_groups/0"
    (gdb) x/s 0x7ffff7ffed39 # result from basename
    0x7ffff7ffed39: "0"
    (gdb) x/s 0xfffffffff7ffed39 # after sign extension
    0xfffffffff7ffed39:     <error: Cannot access memory at address 0xfffffffff7ffed39>

Signed-off-by: Jules Maselbas <jmaselbas@zdiv.net>
---
 lib/sysfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/sysfs.c b/lib/sysfs.c
index 0e763dc..1644e51 100644
--- a/lib/sysfs.c
+++ b/lib/sysfs.c
@@ -9,11 +9,10 @@
  *	SPDX-License-Identifier: GPL-2.0-or-later
  */
 
-#define _GNU_SOURCE
-
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <libgen.h>
 #include <stdarg.h>
 #include <unistd.h>
 #include <errno.h>
-- 
2.44.0

Re: [RESEND PATCH] libpci: sysfs: Fix segmentation fault by including libgen.h

[Martin Mareš]

Hello!

> On a musl-based system (Alpine-linux) the basename(3) function is not defined
> by including string.h with _GNU_SOURCE defined. However basename(3) could be
> defined by including libgen.h.
> 
> On musl this is a problem than can lead to a segmentation fault, as I have
> experienced. This issue is caused by basename(3) function being implicitly
> declared and thus having, implicitly, a return type of int. Which in my case
> caused an erroneous sign extension of a pointer leading to a segmentation
> fault.
> 
> Adding an include for libgen.h sound to me like a proper solution.
> Also by doing so the `_GNU_SOURCE` defined is no longer needed.

It should be fixed by commit 89cb2ae87236604b0e8ededd0fd7d9425c2d8cb6.

Could you please check if it works for you?

				Have a nice fortnight
-- 
Martin `MJ' Mareš                        <mj@ucw.cz>   http://mj.ucw.cz/
United Computer Wizards, Prague, Czech Republic, Europe, Earth, Universe
Sic transit gloria Monday!

Re: [RESEND PATCH] libpci: sysfs: Fix segmentation fault by including libgen.h

[Jules Maselbas]

On Mon May 27, 2024 at 2:33 PM CEST, Martin Mareš wrote:
> Hello!
>
> > On a musl-based system (Alpine-linux) the basename(3) function is not defined
> > by including string.h with _GNU_SOURCE defined. However basename(3) could be
> > defined by including libgen.h.
> > 
> > On musl this is a problem than can lead to a segmentation fault, as I have
> > experienced. This issue is caused by basename(3) function being implicitly
> > declared and thus having, implicitly, a return type of int. Which in my case
> > caused an erroneous sign extension of a pointer leading to a segmentation
> > fault.
> > 
> > Adding an include for libgen.h sound to me like a proper solution.
> > Also by doing so the `_GNU_SOURCE` defined is no longer needed.
>
> It should be fixed by commit 89cb2ae87236604b0e8ededd0fd7d9425c2d8cb6.
>
> Could you please check if it works for you?
Yes, it does work.

Re: [RESEND PATCH] libpci: sysfs: Fix segmentation fault by including libgen.h

[Jules Maselbas]

On Mon May 27, 2024 at 2:33 PM CEST, Martin Mareš wrote:
> Hello!
>
> > On a musl-based system (Alpine-linux) the basename(3) function is not defined
> > by including string.h with _GNU_SOURCE defined. However basename(3) could be
> > defined by including libgen.h.
> > 
> > On musl this is a problem than can lead to a segmentation fault, as I have
> > experienced. This issue is caused by basename(3) function being implicitly
> > declared and thus having, implicitly, a return type of int. Which in my case
> > caused an erroneous sign extension of a pointer leading to a segmentation
> > fault.
> > 
> > Adding an include for libgen.h sound to me like a proper solution.
> > Also by doing so the `_GNU_SOURCE` defined is no longer needed.
>
> It should be fixed by commit 89cb2ae87236604b0e8ededd0fd7d9425c2d8cb6.
>
> Could you please check if it works for you?
nitpick / for the record:
it previously compiled with musl, gcc (13) only generated a warning,
but it should nolonger compile with gcc 14, as the warning is now an error.