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

[Quentin Schulz <quentin.schulz@cherry.de>]

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@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)
> +
> +@pytest.mark.boardspec('sandbox')
> +@pytest.mark.requiredtool('dtc')
> +@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?

Looks good to me otherwise.

Cheers,
Quentin