| Message ID | embe01a23a-c839-406f-b9f0-9b9dbd4bc5c2@e0f6f3eb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kmod/depmod: explicitly include the header for basename() | expand |
On Mon, Apr 08, 2024 at 08:22:01PM +0000, Laurent Bercot wrote: > Hello list, > >(Please Cc: me on the replies.) > > Recent versions of the musl libc declare basename() (and dirname()) >exclusively in <libgen.h>, as specified by POSIX. If this header is >not properly included, when building kmod with musl, basename() is an >unknown symbol and will be assumed as returning int, which causes >problems as soon as kmod performs a printf("%s", basename(argv[0])), >e.g. in kmod_help(). (On x86_64, int is 32 bit, so the pointer address >is truncated, which causes a segfault on access.) > > Simply including libgen.h wherever basename() is used, i.e. depmod.c that is the wrong basename though. https://man7.org/linux/man-pages/man3/basename.3.html There are two different versions of basename() - the POSIX version described above, and the GNU version, which one gets after #define _GNU_SOURCE /* See feature_test_macros(7) */ #include <string.h> The GNU version never modifies its argument, and returns the empty string when path has a trailing slash, and in particular also when it is "/". There is no GNU version of dirname(). We only ever use and want the gnu behavior, that doesn't modify the argument. Which is the proper thing to do for a basename() implementation. There's a pending patch I need to review: https://github.com/kmod-project/kmod/pull/32 does that fix it for you? thanks Lucas De Marchi >and kmod.c, fixes the issue. It will print warnings because you store >the result in a const char *, but these are harmless and can be fixed >later. > > None of the kmod files seems to use dirname(), but several of them use >dirname as a symbol, including depmod.c, where it will shadow the libc's >dirname symbol. This does not cause a problem right now, but it might be >a good idea to rename the dirname variables at some point. > > >diff -rNU3 kmod-32.old/tools/depmod.c kmod-32/tools/depmod.c >--- kmod-32.old/tools/depmod.c 2023-12-06 16:34:31.000000000 +0100 >+++ kmod-32/tools/depmod.c 2024-04-08 20:55:03.998592078 +0200 >@@ -22,6 +22,7 @@ > #include <dirent.h> > #include <errno.h> > #include <getopt.h> >+#include <libgen.h> > #include <limits.h> > #include <regex.h> > #include <stdio.h> >diff -rNU3 kmod-32.old/tools/kmod.c kmod-32/tools/kmod.c >--- kmod-32.old/tools/kmod.c 2024-02-20 23:10:55.000000000 +0100 >+++ kmod-32/tools/kmod.c 2024-04-08 21:55:03.888577992 +0200 >@@ -19,6 +19,7 @@ > > #include <errno.h> > #include <getopt.h> >+#include <libgen.h> > #include <stdio.h> > #include <stdlib.h> > #include <string.h> >
>We only ever use and want the gnu behavior, that doesn't modify the >argument. Which is the proper thing to do for a basename() >implementation. I agree, but as I just stated in my comment to the patch below, if you don't want POSIX basename() behaviour, you probably should not use basename(). GNU should have named its own (admittedly better) version differently; it's just less error-prone to have different symbols for different things, and I don't think it's a good idea to tie a project to this particular GNU bit of carelessness. >There's a pending patch I need to review: >https://github.com/kmod-project/kmod/pull/32 > >does that fix it for you? Absolutely, this patch is way better than mine and I should have thought of looking for something similar before submitting mine. Please merge Khem's patch. :) -- Laurent
On Tue, Apr 09, 2024 at 04:16:48PM +0000, Laurent Bercot wrote: > GNU should have named its own (admittedly better) version agreed >differently; it's just less error-prone to have different symbols for >different things, and I don't think it's a good idea to tie a project to >this particular GNU bit of carelessness. agreed, but that's not really up to each project. POSIX/libc should have provided the better alternative, because the current one from libgen.h makes no sense for what the function does. I understand why an implementation of dirname() could modify the buffer passed in, but not basename(). If musl provided a better one with a different name, I'd be happy to alias basename to it. If I cared enough, I'd request a "sane_basename() / basename_dont_be_evil()" to be included in POSIX. I don't care enough, but as I said it's surprising libc developers don't care neither and instead we keep patching all code on earth to duplicate what should really be the normal behavior of such a function from libc. My problem with s/basename/gnu_basename/ is that eventually we will forget and just use basename in future. And break musl again. And it may even be just a silent error, modifying a buffer that shouldn't really be modified. given that so far we never needed dirname() from libgen.h, we could just ban it. Something like: diff --git a/Makefile.am b/Makefile.am index e2e2411..6ca787a 100644 --- a/Makefile.am +++ b/Makefile.am @@ -17,6 +17,7 @@ export GCC_COLORS AM_CPPFLAGS = \ -include $(top_builddir)/config.h \ + -I$(top_srcdir)/shared/missing \ -I$(top_srcdir) \ -DSYSCONFDIR=\""$(sysconfdir)"\" \ -DDISTCONFDIR=\""$(distconfdir)"\" \ @@ -47,6 +48,7 @@ noinst_LTLIBRARIES = shared/libshared.la shared_libshared_la_SOURCES = \ shared/macro.h \ shared/missing.h \ + shared/missing/libgen.h \ shared/array.c \ shared/array.h \ shared/hash.c \ diff --git a/shared/missing/libgen.h b/shared/missing/libgen.h new file mode 100644 index 0000000..3c78586 --- /dev/null +++ b/shared/missing/libgen.h @@ -0,0 +1 @@ +#error "libgen.h can't be included in this project. Did you mean to use string.h?" at least on my system it seems libgen.h is never implicitely included. Lucas De Marchi > > >>There's a pending patch I need to review: >>https://github.com/kmod-project/kmod/pull/32 >> >>does that fix it for you? > > Absolutely, this patch is way better than mine and I should have >thought >of looking for something similar before submitting mine. Please merge >Khem's patch. :) > >-- > Laurent >
diff -rNU3 kmod-32.old/tools/depmod.c kmod-32/tools/depmod.c --- kmod-32.old/tools/depmod.c 2023-12-06 16:34:31.000000000 +0100 +++ kmod-32/tools/depmod.c 2024-04-08 20:55:03.998592078 +0200 @@ -22,6 +22,7 @@ #include <dirent.h> #include <errno.h> #include <getopt.h> +#include <libgen.h> #include <limits.h> #include <regex.h> #include <stdio.h> diff -rNU3 kmod-32.old/tools/kmod.c kmod-32/tools/kmod.c --- kmod-32.old/tools/kmod.c 2024-02-20 23:10:55.000000000 +0100 +++ kmod-32/tools/kmod.c 2024-04-08 21:55:03.888577992 +0200 @@ -19,6 +19,7 @@ #include <errno.h> #include <getopt.h> +#include <libgen.h> #include <stdio.h> #include <stdlib.h> #include <string.h>
Hello list, (Please Cc: me on the replies.) Recent versions of the musl libc declare basename() (and dirname()) exclusively in <libgen.h>, as specified by POSIX. If this header is not properly included, when building kmod with musl, basename() is an unknown symbol and will be assumed as returning int, which causes problems as soon as kmod performs a printf("%s", basename(argv[0])), e.g. in kmod_help(). (On x86_64, int is 32 bit, so the pointer address is truncated, which causes a segfault on access.) Simply including libgen.h wherever basename() is used, i.e. depmod.c and kmod.c, fixes the issue. It will print warnings because you store the result in a const char *, but these are harmless and can be fixed later. None of the kmod files seems to use dirname(), but several of them use dirname as a symbol, including depmod.c, where it will shadow the libc's dirname symbol. This does not cause a problem right now, but it might be a good idea to rename the dirname variables at some point.