[PATCH v1 1/2] tools: mkimage: fix get_basename crash on paths with dotted directories

[Aristo Chen]

On Fri, May 22, 2026 at 12:17 AM Marek Vasut <marek.vasut at mailbox.org> wrote:
>
> On 5/21/26 6:12 PM, Quentin Schulz wrote:
>
> Hello Quentin,
>
> >>> +    p = strrchr(start, '.');
> >>> +    end = p ? p : start + strlen(start);
> >>>       len = end - start;
> >>>       if (len >= size)
> >>>           len = size - 1;
> >>
> >> Why not call basename(3) directly in here ? Why reimplement it ?
> >
> > """
> > Warning: there are two different functions basename() - see below.
> >
> > [...]
> >
> > Notes
> >
> > 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().
> >
> > With glibc, one gets the POSIX version of basename() when <libgen.h> is
> > included, and the GNU version otherwise.
> >
> > Bugs
> >
> > In the glibc implementation of the POSIX versions of these functions
> > they modify their argument, and segfault when called with a static
> > string like "/usr/". Before glibc 2.2.1, the glibc version of dirname()
> > did not correctly handle pathnames with trailing '/' characters, and
> > generated a segfault if given a NULL argument.
> > """
> >
> > Not very reassuring, tbh.
>
> Writing our own variant with its own set of bugs is worse than using a
> common implementation which has the bugs removed over time due to effort
> of the various users.
>
> If basename(3) is not an option, is there another common alternative ?

I agree with the principle, but IMO get_basename() is not really a
reimplementation of basename(3). It does two things, and basename(3)
covers only one of them.

get_basename() strips the directory and the extension. Per its own comment:

"arch/arm/dts/sun7i-a20-bananapro.dtb"
becomes "sun7i-a20-bananapro"

basename(3) on that path returns "sun7i-a20-bananapro.dtb"; the
extension stays on. So basename(3) would replace only the directory
half, the strrchr('/') scan. The extension half, the strrchr('.') scan
that this crash actually came from, still has to be done by hand.

There is no libc one-shot for "basename without the extension".
U-Boot's own host tooling reflects that: binman does it in two steps
wherever it needs a stem, for example in tools/binman/control.py:

os.path.splitext(os.path.basename(item))[0]
basename() followed by splitext(), because no single call does both.

basename(3) also carries the two-version problem Quentin already
quoted. For mkimage specifically: tools/Makefile builds every host
tool with -D_GNU_SOURCE, and fit_image.c includes <string.h> but not
<libgen.h>. That combination resolves to the GNU basename(), the
const-safe one that leaves its argument alone. But the selection is
implicit: if anyone later adds #include <libgen.h> to this file,
basename() silently becomes the POSIX version that modifies its
argument and segfaults on string literals. That is a sharper footgun
than the small hand-rolled helper, and it would sit in the very
function we are trying to make safe.

So my preference is to keep get_basename() hand-rolled. This patch is
not new logic; it only bounds the existing strrchr('.') scan to the
part after the last '/', which is the minimal fix for the
negative-length crash.

If you would still rather use basename(3), the least fragile form is
GNU basename() for the leaf, then strrchr('.') on that leaf to drop
the extension, with a comment pinning the GNU-version dependency so
nobody includes <libgen.h> later. Either way I will send a v2 (there
are review comments on patch 2/2 to address as well), so just let me
know which direction you prefer for get_basename() and I will include
it there.

Thanks,
Aristo