[libcamera-devel] [PATCH v2] libcamera: controls: validate all ControlInfo values

[Kieran Bingham]

Quoting Christian Rauch via libcamera-devel (2022-09-21 21:31:48)
> ControlInfoMap::validate only checks the 'min' type against the ControlId
> type. Extend this with checks against the 'max' type and the 'def' type,
> if a default is specified. This forces the min/max bounds to have the same
> type as the controlled value, but leaves the default optional.
> 
> Signed-off-by: Christian Rauch <Rauch.Christian at gmx.de>
> ---
>  src/libcamera/controls.cpp | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index bc3db4f6..e7251507 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -701,6 +701,28 @@ bool ControlInfoMap::validate()
>                                       ? ControlTypeInteger32 : id->type();
>                 const ControlInfo &info = ctrl.second;
> 
> +               if (!(info.min().isArray() == info.max().isArray() &&
> +                     info.min().numElements() == info.max().numElements())) {

Does numElements work even if the type is not an array? (i.e. will it
return '1'?)

If so - that could perhaps be simplified to just:
		if (info.min().numElements() != info.max().numElements())



> +                       LOG(Controls, Error)
> +                               << "Control " << utils::hex(id->id())
> +                               << " range must have the same dimension.";
> +                       return false;
> +               }
> +
> +               if (info.min().type() != info.max().type()) {

This will already enforce the types are the same. But perhaps the type
checking should be before the size checking above.

> +                       LOG(Controls, Error)
> +                               << "Control " << utils::hex(id->id())
> +                               << " range types mismatch";
> +                       return false;
> +               }
> +
> +               if (info.def().type() != ControlTypeNone && (info.min().type() != info.def().type())) {

Perhaps neater over two lines, and I believe the second parenthesis
isn't required:

		if (info.def().type() != ControlTypeNone &&
		    info.min().type() != info.def().type()) {



> +                       LOG(Controls, Error)
> +                               << "Control " << utils::hex(id->id())

Do we have access to any human readable string here instead of a hex of
the id()?

If so - It would be better to use that (but it may not be available)

With those considered:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +                               << " default value and info type mismatch";
> +                       return false;
> +               }
> +
>                 if (info.min().type() != rangeType) {
>                         LOG(Controls, Error)
>                                 << "Control " << utils::hex(id->id())
> --
> 2.34.1
>