| Message ID | 20240521080519.7833-1-jmaselbas@zdiv.net |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] libpci: sysfs: Fix segmentation fault by including libgen.h | expand |
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
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.
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.
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>
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(-)