[libcamera-devel] [PATCH v1 2/4] test: v4l2_compat: Enable test with ASan

[Umang Jain]

Hi Laurent

Thank you for this patch

On 12/22/22 6:31 AM, Laurent Pinchart via libcamera-devel wrote:
> When libcamera is compiled with the address sanitizer enabled, the
> v4l2_compat test generates failures in the link order runtime check, as
> the host v4l2-ctl and v4l2-compliance tools are not (generally) linked
> to ASan. For this reason, the test is disabled, which sadly shrinks test
> coverage.
>
> Fix this by loading the ASan runtime using LD_PRELOAD. This needs to be
> done from within the v4l2_compat_test.py Python script, as the Python
> interpreter itself leaks memory and would cause test failures if run
> with ASan.
>
> To LD_PRELOAD the ASan runtime, the path to the binary needs to be
> known. gcc gives us a generic way to get the path, but that doesn't work
> with clang as the ASan runtime file name depends on the clang version
> and target architecture. We thus have to keep the v4l2_compat test
> disabled with ASan is enabled and libcamera is compiled with clang.
s/disabled with ASan/disabled when ASan/  probably?

Oh dear - quite a case to deal with`
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>   test/meson.build                     | 16 ++++++++++++++++
>   test/v4l2_compat/meson.build         | 21 ++++++++++++++-------
>   test/v4l2_compat/v4l2_compat_test.py | 18 +++++++++++++-----
>   3 files changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/test/meson.build b/test/meson.build
> index 05a54d5cad2f..19726f37421d 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -7,6 +7,22 @@ endif
>   
>   test_enabled = true
>   
> +# When ASan is enabled, find the path to the ASan runtime needed by multiple
> +# tests. This currently works with gcc only, as clang uses different file names
> +# depending on the compiler version and target architecture.
> +asan_enabled = false
> +asan_runtime_missing = false
> +
> +if get_option('b_sanitize').contains('address')
> +    asan_enabled = true
> +
> +    if cc.get_id() == 'gcc'
> +        asan_runtime = run_command(cc, '-print-file-name=libasan.so', check : true).stdout().strip()
> +    else
> +        asan_runtime_missing = true
> +    endif
> +endif
> +
>   subdir('libtest')
>   
>   subdir('camera')
> diff --git a/test/v4l2_compat/meson.build b/test/v4l2_compat/meson.build
> index 10c4675286b3..da2e7a6d9045 100644
> --- a/test/v4l2_compat/meson.build
> +++ b/test/v4l2_compat/meson.build
> @@ -4,19 +4,26 @@ if not is_variable('v4l2_compat')
>       subdir_done()
>   endif
>   
> -# If ASan is enabled, the link order runtime check will fail as v4l2-ctl and
> -# v4l2-compliance are not linked to ASan. Skip the test in that case.
> -#
> -# TODO: Find a way to LD_PRELOAD the ASan dynamic library instead, in a
> -# cross-platform way with support for both gcc and clang.
> +# If ASan is enabled but the ASan runtime shared library missing,
> +# v4l2_compat_test.py won't be able to LD_PRELOAD it, resulting in a link order
> +# runtime check failure as v4l2-ctl and v4l2-compliance are not linked to ASan.
> +# Skip the test in that case.
>   
> -if get_option('b_sanitize').contains('address')
> +if asan_runtime_missing
> +    warning('Unable to get path to ASan runtime, v4l2_compat test disabled')
>       subdir_done()
>   endif
>   
>   v4l2_compat_test = files('v4l2_compat_test.py')
> +v4l2_compat_args = []
> +
> +if asan_enabled
> +    v4l2_compat_args += ['-s', asan_runtime]
> +endif
> +
> +v4l2_compat_args += [v4l2_compat]
>   
>   test('v4l2_compat_test', v4l2_compat_test,
> -     args : v4l2_compat,
> +     args : v4l2_compat_args,
>        suite : 'v4l2_compat',
>        timeout : 60)

Looks good till here, taking a break to review the python diffs below
> diff --git a/test/v4l2_compat/v4l2_compat_test.py b/test/v4l2_compat/v4l2_compat_test.py
> index a77585fc2f49..bd89d4962c16 100755
> --- a/test/v4l2_compat/v4l2_compat_test.py
> +++ b/test/v4l2_compat/v4l2_compat_test.py
> @@ -57,8 +57,8 @@ def extract_result(result):
>       return ret
>   
>   
> -def test_v4l2_compliance(v4l2_compliance, v4l2_compat, device, base_driver):
> -    ret, output = run_with_stdout(v4l2_compliance, '-s', '-d', device, env={'LD_PRELOAD': v4l2_compat})
> +def test_v4l2_compliance(v4l2_compliance, ld_preload, device, base_driver):
> +    ret, output = run_with_stdout(v4l2_compliance, '-s', '-d', device, env={'LD_PRELOAD': ld_preload})
>       if ret < 0:
>           output.append(f'Test for {device} terminated due to signal {signal.Signals(-ret).name}')
>           return TestFail, output
> @@ -82,13 +82,21 @@ def main(argv):
>       parser = argparse.ArgumentParser()
>       parser.add_argument('-a', '--all', action='store_true',
>                           help='Test all available cameras')
> +    parser.add_argument('-s', '--sanitizer', type=str,
> +                        help='Path to the address sanitizer (ASan) runtime')
>       parser.add_argument('-v', '--verbose', action='store_true',
>                           help='Make the output verbose')
>       parser.add_argument('v4l2_compat', type=str,
>                           help='Path to v4l2-compat.so')
>       args = parser.parse_args(argv[1:])
>   
> -    v4l2_compat = args.v4l2_compat
> +    # Compute the LD_PRELOAD value by first loading ASan (if specified) and
> +    # then the V4L2 compat layer.
> +    ld_preload = []
> +    if args.sanitizer:
> +        ld_preload.append(args.sanitizer)
> +    ld_preload.append(args.v4l2_compat)
> +    ld_preload = ':'.join(ld_preload)
>   
>       v4l2_compliance = shutil.which('v4l2-compliance')
>       if v4l2_compliance is None:
> @@ -118,7 +126,7 @@ def main(argv):
>       failed = []
>       drivers_tested = {}
>       for device in dev_nodes:
> -        ret, out = run_with_stdout(v4l2_ctl, '-D', '-d', device, env={'LD_PRELOAD': v4l2_compat})
> +        ret, out = run_with_stdout(v4l2_ctl, '-D', '-d', device, env={'LD_PRELOAD': ld_preload})
>           if ret < 0:
>               failed.append(device)
>               print(f'v4l2-ctl failed on {device} with v4l2-compat')
> @@ -144,7 +152,7 @@ def main(argv):
>               continue
>   
>           print(f'Testing {device} with {driver} driver... ', end='')
> -        ret, msg = test_v4l2_compliance(v4l2_compliance, v4l2_compat, device, driver)
> +        ret, msg = test_v4l2_compliance(v4l2_compliance, ld_preload, device, driver)
>           if ret == TestFail:
>               failed.append(device)
>               print('failed')

Looks good to me and adequately  documented

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>