Hi Quentin, On Thu, May 21, 2026 at 5:10 PM Quentin Schulz <[email protected]> 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 <[email protected]> > > --- > > 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) > > + > > [email protected]('sandbox') > > [email protected]('dtc') > > [email protected]('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
