[PATCH v1 2/2] test/py: cover get_basename crash on paths with dotted directories

[Aristo Chen]

Hi Quentin,

On Thu, May 21, 2026 at 5:10 PM Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> Hi Aristo,
>
> On 5/21/26 4:35 AM, Aristo Chen wrote:
> > Add a parametrized regression test for the fix in the previous commit.
> > The test invokes mkimage in auto-FIT mode (-f auto) with a -b argument
> > whose directory component contains a '.' and whose leaf either lacks an
> > extension or is a plain identifier. Before the fix these inputs caused
> > get_basename() to compute a negative length and segfault inside memcpy.
> > The test asserts that mkimage exits successfully and that the fdt
> > sub-image description matches the expected stripped basename, covering
> > "./mydt", "./sub.d/leaf", and "./a.b/c". A control input of "./mydt.dtb"
> > is also exercised to confirm normal extension stripping still works.
> >
> > Signed-off-by: Aristo Chen <aristo.chen at canonical.com>
> > ---
> >   test/py/tests/test_fit_mkimage_validate.py | 55 ++++++++++++++++++++++
> >   1 file changed, 55 insertions(+)
> >
> > diff --git a/test/py/tests/test_fit_mkimage_validate.py b/test/py/tests/test_fit_mkimage_validate.py
> > index 170b2a8cbbb..0a1cc5963a6 100644
> > --- a/test/py/tests/test_fit_mkimage_validate.py
> > +++ b/test/py/tests/test_fit_mkimage_validate.py
> > @@ -103,3 +103,58 @@ def test_fit_invalid_default_config(ubman):
> >
> >       assert result.returncode != 0, "mkimage should fail due to missing default config"
> >       assert re.search(r"Default configuration '.*' not found under /configurations", result.stderr)
> > +
> > + at pytest.mark.boardspec('sandbox')
> > + at pytest.mark.requiredtool('dtc')
> > + at pytest.mark.parametrize('dtb_relpath,expected_desc', [
> > +    # Crash triggers: last '.' precedes last '/', or leaf has no extension.
> > +    ('./mydt',       'mydt'),
> > +    ('./sub.d/leaf', 'leaf'),
> > +    ('./a.b/c',      'c'),
> > +    # Control case: extension lives in the leaf, no dotted directory.
> > +    ('./mydt.dtb',   'mydt'),
> > +])
> > +def test_fit_auto_basename_dotted_directory(ubman, dtb_relpath, expected_desc):
> > +    """Regression test: mkimage -f auto must not crash when a -b path has a
> > +    '.' in its directory portion.
> > +
> > +    Before the fix, get_basename() in tools/fit_image.c searched the whole
> > +    path for both the last '/' and the last '.'. When the '.' fell before
> > +    the '/', the computed length went negative and was passed unchanged to
> > +    memcpy(), which segfaulted. This test exercises three crashing paths
> > +    plus one control input.
> > +    """
> > +    build_dir = ubman.config.build_dir
> > +    kernel = fit_util.make_kernel(ubman, 'kernel.bin', 'kernel')
> > +    itb_fname = fit_util.make_fname(ubman, 'auto_basename.itb')
> > +
> > +    # Materialize the dtb at the requested relative path inside build_dir.
> > +    dtb_abs = os.path.join(build_dir, dtb_relpath)
> > +    os.makedirs(os.path.dirname(dtb_abs), exist_ok=True)
> > +    with open(dtb_abs, 'wb') as f:
> > +        f.write(b'dummy')
> > +
> > +    mkimage = os.path.join(build_dir, 'tools/mkimage')
> > +    cmd = [mkimage, '-f', 'auto',
> > +           '-A', 'arm', '-O', 'linux', '-T', 'kernel', '-C', 'none',
> > +           '-a', '0x80000000', '-e', '0x80000000', '-n', 'test',
> > +           '-d', kernel,
> > +           '-b', dtb_relpath,
> > +           itb_fname]
> > +    # Run with cwd=build_dir so the relative path resolves the same way
> > +    # the bug originally reproduced.
> > +    result = subprocess.run(cmd, capture_output=True, text=True,
> > +                            cwd=build_dir)
>
> Considering we set cwd to build_dir, can't we simply use ./tools/mkimage
> instead of prepending build_dir to it? It's unclear to me if it's an
> absolute path, but I'm assuming it is otherwise the test wouldn't run.
>
> > +
> > +    assert result.returncode == 0, (
> > +        f"mkimage crashed or failed on -b {dtb_relpath!r}: "
> > +        f"rc={result.returncode}\nstdout:\n{result.stdout}\n"
> > +        f"stderr:\n{result.stderr}"
> > +    )
> > +    # The fdt sub-image description is set from get_basename(); confirm it
> > +    # matches the expected stripped basename.
> > +    assert re.search(rf"Image 1 \(fdt-1\)\s+Description:\s+{re.escape(expected_desc)}\b",
> > +                     result.stdout), (
> > +        f"Expected fdt-1 description {expected_desc!r} in mkimage output, "
> > +        f"got:\n{result.stdout}"
> > +    )
>
> I can't help but wonder if we shouldn't make this less dependent on
> mkimage's console output (and thus requiring a regex). FIT data
> structure is simply a device tree so why not parse it to look for the
> description property of the /images/fdt-1 node?
>
> What do you think?

Thanks for the feedback! That makes sense to me, and I will prepare a v2

>
> Looks good to me otherwise.
>
> Cheers,
> Quentin

Best regards,
Aristo