[PATCH] kmod/depmod: explicitly include the header for basename()

[PATCH] kmod/depmod: explicitly include the header for basename()

[Laurent Bercot]

  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.


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>

Re: [PATCH] kmod/depmod: explicitly include the header for basename()

[Lucas De Marchi]

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>
>

Re: [PATCH] kmod/depmod: explicitly include the header for basename()

[Laurent Bercot]

>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

Re: [PATCH] kmod/depmod: explicitly include the header for basename()

[Lucas De Marchi]

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
>