Hi Aristo,
On 2026-10-11T15:21:13, Aristo Chen <[email protected]> wrote:
> test: py: cover invalid external data properties in fit_import_data()
>
> Build an external-data FIT, then point one image's data-offset far past
> the end of the file and, separately, make its data-size run a few bytes
> past the end, re-processing the result with mkimage -F each time. Both
> must be rejected with a diagnostic naming that image. Previously the
> first crashed mkimage and the second silently imported the bytes beyond
> the end of the file as image data.
>
> Also check that a FIT whose images are all empty, which mkimage -E -p
> writes with data-position past the end of the file, still re-processes,
> since an empty region has nothing to read.
>
> Finally, now that the import reads the properties through the FIT
> accessors, check that an empty data-offset or data-size is skipped like
> a missing property, so that the hashing stage reports the image instead
> of the import reading it from the bytes that follow it.
I don't think this is quite right. Before patch 4 the import used
fdtdec_get_int(), which already returns the default for a property
shorter than an int (see the 'len >= sizeof(int)' check in
lib/fdtdec_common.c). So the import never read an empty property, and
patch 4's commit message says as much ('as it already did for a
shorter one'). What this test actually guards is patch 2. Once the
import skips the image, the hashing stage in tools/image-host.c calls
fit_image_get_data(), which without patch 2 would read the following
FDT tag as the offset or size and carry on. Please can you reword this
paragraph to say so?
>
> Signed-off-by: Aristo Chen <[email protected]>
>
> test/py/tests/test_fit_import_data.py | 91 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 90 insertions(+), 1 deletion(-)
> diff --git a/test/py/tests/test_fit_import_data.py
> b/test/py/tests/test_fit_import_data.py
> @@ -113,3 +118,87 @@ def test_fit_import_data_missing_offset(ubman):
> + # An empty property must be skipped like a missing one, so that the
> + # hashing stage reports the image, rather than read from the bytes
> + # that follow it, which would import a few bytes or the wrong data
> + for prop in ('data-offset', 'data-size'):
Same here: 'import a few bytes' was never possible for an empty property.
The case that patch 4 does change in fit_import_data() is a property
longer than one cell. fdtdec_get_int() accepts it and uses the first
cell, but the accessors now reject it. Please can you add a two-cell
data-offset and data-size (e.g. fdtput -t i ... 0 0) so that the
import change itself is covered? The test name and docstring could
then be widened from 'shorter than one cell' to 'not exactly one
cell'.
Regards,
Simon
