[libcamera-devel] [PATCH v5] libcamera: v4l2_device: Workaround faulty control menus

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Nov 24, 2022 at 02:45:53PM +0000, Kieran Bingham via libcamera-devel wrote:
> Some UVC cameras have been identified that can provide V4L2 menu
> controls without any menu items.
> 
> This leads to a segfault where we try to construct a
> ControlInfo(Span<>,default) with an empty span.
> 
> Convert the v4l2ControlInfo and v4l2MenuControlInfo helper functions to
> return std::optional<ControlInfo> to be able to account in the caller if
> the control is valid, and only add acceptable controls to the supported
> control list.
> 
> Menu controls without a list of menu items are no longer added as a
> valid control and a warning is logged.
> 
> This also fixes a potential crash that would have occured in the
> unlikely event that a ctrl.minimum was set to less than 0.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=167
> Reported-by: Marian Buschsieweke <marian.buschsieweke at ovgu.de>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
> 
> As I posted two v3 patches, I've gone straight to v5 on this.

:-D

> Now there's only the common path error report, and a couple of minor
> text fixes.
> 
> 
>  include/libcamera/internal/v4l2_device.h |  4 ++--
>  src/libcamera/v4l2_device.cpp            | 28 +++++++++++++++++++-----
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index 75304be11f77..50d4adbc5f2b 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -70,8 +70,8 @@ protected:
>  private:
>  	static ControlType v4l2CtrlType(uint32_t ctrlType);
>  	static std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl);
> -	ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);
> -	ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);
> +	std::optional<ControlInfo> v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);
> +	std::optional<ControlInfo> v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);
>  
>  	void listControls();
>  	void updateControls(ControlList *ctrls,
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index c17b323f8af0..57a88d96b12c 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -529,7 +529,7 @@ std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &
>   * \param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control
>   * \return A ControlInfo that represents \a ctrl
>   */
> -ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
> +std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
>  {
>  	switch (ctrl.type) {
>  	case V4L2_CTRL_TYPE_U8:
> @@ -566,14 +566,14 @@ ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
>   *
>   * \return A ControlInfo that represents \a ctrl
>   */
> -ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> +std::optional<ControlInfo> V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
>  {
>  	std::vector<ControlValue> indices;
>  	struct v4l2_querymenu menu = {};
>  	menu.id = ctrl.id;
>  
>  	if (ctrl.minimum < 0)
> -		return ControlInfo();
> +		return std::nullopt;
>  
>  	for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index) {
>  		menu.index = index;
> @@ -583,6 +583,14 @@ ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ct
>  		indices.push_back(index);
>  	}
>  
> +	/*
> +	 * Some faulty UVC devices are known to return an empty menu control.
> +	 * Controls without a menu option can not be set, or read, so they are
> +	 * not exposed.
> +	 */
> +	if (indices.size() == 0)
> +		return std::nullopt;
> +
>  	return ControlInfo(indices,
>  			   ControlValue(static_cast<int32_t>(ctrl.default_value)));
>  }
> @@ -631,7 +639,17 @@ void V4L2Device::listControls()
>  		controlIdMap_[ctrl.id] = controlIds_.back().get();
>  		controlInfo_.emplace(ctrl.id, ctrl);
>  
> -		ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));
> +		std::optional<ControlInfo> info = v4l2ControlInfo(ctrl);
> +
> +		if (!info) {
> +			LOG(V4L2, Error)
> +				<< "Control " << ctrl.name
> +				<< " cannot be registered";
> +
> +			continue;
> +		}
> +
> +		ctrls.emplace(controlIds_.back().get(), *info);
>  	}
>  
>  	controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);
> @@ -670,7 +688,7 @@ void V4L2Device::updateControlInfo()
>  			continue;
>  		}
>  
> -		info = v4l2ControlInfo(ctrl);
> +		info = *v4l2ControlInfo(ctrl);
>  	}
>  }
>  

-- 
Regards,

Laurent Pinchart