[libcamera-devel,v3] libcamera: v4l2_device: Workaround faulty control menus

17896 diff mbox series

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@ovgu.de>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/internal/v4l2_device.h |  4 +--
 src/libcamera/v4l2_device.cpp            | 31 ++++++++++++++++++++----
 2 files changed, 28 insertions(+), 7 deletions(-)

Hi Kieran,

Thank you for the patch.

On Thu, Nov 24, 2022 at 01:03:08PM +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@ovgu.de>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_device.h |  4 +--
>  src/libcamera/v4l2_device.cpp            | 31 ++++++++++++++++++++----
>  2 files changed, 28 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..5dd51037960d 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,17 @@ ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ct
>  		indices.push_back(index);
>  	}
>  
> +	/*
> +	 * Some faulty UVC drivers are known to return an empty menu control.

s/drivers/devices/ ?

> +	 * Controls without a menu option can not be set, or read, so they are
> +	 * not exposed.
> +	 */
> +	if (indices.size() == 0) {
> +		LOG(V4L2, Warning)
> +			<< "Menu control '" << ctrl.name << "' has no entries";

Would you consider this a driver bug ? If so a warning sounds fine, and
we should also fix the driver. If not, maybe we should demote the
warning message to a debug message ?

> +		return std::nullopt;
> +	}
> +
>  	return ControlInfo(indices,
>  			   ControlValue(static_cast<int32_t>(ctrl.default_value)));
>  }
> @@ -631,7 +642,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.";

s/registered./registered/

But I would drop this as we already log a message in
v4l2MenuControlInfo() (or you could drop the message there instead).

> +
> +			continue;
> +		}
> +
> +		ctrls.emplace(controlIds_.back().get(), *info);
>  	}
>  
>  	controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);
> @@ -670,7 +691,7 @@ void V4L2Device::updateControlInfo()
>  			continue;
>  		}
>  
> -		info = v4l2ControlInfo(ctrl);
> +		info = *v4l2ControlInfo(ctrl);
>  	}
>  }
>

Quoting Laurent Pinchart (2022-11-24 14:07:57)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Nov 24, 2022 at 01:03:08PM +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@ovgu.de>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  include/libcamera/internal/v4l2_device.h |  4 +--
> >  src/libcamera/v4l2_device.cpp            | 31 ++++++++++++++++++++----
> >  2 files changed, 28 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..5dd51037960d 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,17 @@ ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ct
> >               indices.push_back(index);
> >       }
> >  
> > +     /*
> > +      * Some faulty UVC drivers are known to return an empty menu control.
> 
> s/drivers/devices/ ?

Ack.

> 
> > +      * Controls without a menu option can not be set, or read, so they are
> > +      * not exposed.
> > +      */
> > +     if (indices.size() == 0) {
> > +             LOG(V4L2, Warning)
> > +                     << "Menu control '" << ctrl.name << "' has no entries";
> 
> Would you consider this a driver bug ? If so a warning sounds fine, and
> we should also fix the driver. If not, maybe we should demote the
> warning message to a debug message ?

No - I understand that it's a firmware bug on the UVC device. The UVC
driver exposes a control, and then generates the menu options available
from a bitmask.  My *assumption* is that this device did not set the
bitmask, so there were no menu options available.

I don't think that can be fixed in uvcvideo driver - if there's no
supported option for the control, it's ... just not a useful control.

Even if we 'fix' the uvcvideo driver to not expose menu controls that
have no menu - they are already out there in the wild - and we thus need
to support this issue here.

If this happens on other occasions, then it could be a driver bug, so
I'd like to kee the error as an explicit warning.


> > +             return std::nullopt;
> > +     }
> > +
> >       return ControlInfo(indices,
> >                          ControlValue(static_cast<int32_t>(ctrl.default_value)));
> >  }
> > @@ -631,7 +642,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.";
> 
> s/registered./registered/
> 
> But I would drop this as we already log a message in
> v4l2MenuControlInfo() (or you could drop the message there instead).

I added this at the request of Jacopo on the previous version, and I
agree, because at this point it's an Error - and it catches more error
paths than just the one highlighted above which would otherwise not
report a message.


> 
> > +
> > +                     continue;
> > +             }
> > +
> > +             ctrls.emplace(controlIds_.back().get(), *info);
> >       }
> >  
> >       controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);
> > @@ -670,7 +691,7 @@ void V4L2Device::updateControlInfo()
> >                       continue;
> >               }
> >  
> > -             info = v4l2ControlInfo(ctrl);
> > +             info = *v4l2ControlInfo(ctrl);
> >       }
> >  }
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Hi Kieran,

On Thu, Nov 24, 2022 at 02:15:17PM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-11-24 14:07:57)
> > On Thu, Nov 24, 2022 at 01:03:08PM +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@ovgu.de>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  include/libcamera/internal/v4l2_device.h |  4 +--
> > >  src/libcamera/v4l2_device.cpp            | 31 ++++++++++++++++++++----
> > >  2 files changed, 28 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..5dd51037960d 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,17 @@ ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ct
> > >               indices.push_back(index);
> > >       }
> > >  
> > > +     /*
> > > +      * Some faulty UVC drivers are known to return an empty menu control.
> > 
> > s/drivers/devices/ ?
> 
> Ack.
> 
> > 
> > > +      * Controls without a menu option can not be set, or read, so they are
> > > +      * not exposed.
> > > +      */
> > > +     if (indices.size() == 0) {
> > > +             LOG(V4L2, Warning)
> > > +                     << "Menu control '" << ctrl.name << "' has no entries";
> > 
> > Would you consider this a driver bug ? If so a warning sounds fine, and
> > we should also fix the driver. If not, maybe we should demote the
> > warning message to a debug message ?
> 
> No - I understand that it's a firmware bug on the UVC device. The UVC
> driver exposes a control, and then generates the menu options available
> from a bitmask.  My *assumption* is that this device did not set the
> bitmask, so there were no menu options available.
> 
> I don't think that can be fixed in uvcvideo driver - if there's no
> supported option for the control, it's ... just not a useful control.

The uvcvideo driver could skip the control in that case. It would be
less confusing for applications in general.

> Even if we 'fix' the uvcvideo driver to not expose menu controls that
> have no menu - they are already out there in the wild - and we thus need
> to support this issue here.

Absolutely, I'm fine with this patch.

> If this happens on other occasions, then it could be a driver bug, so
> I'd like to kee the error as an explicit warning.

A warning printed every time such devices are used bothers me a bit. If
we update the uvcvideo driver to skip the control in that case, I'd be
fine with a warning message.

> > > +             return std::nullopt;
> > > +     }
> > > +
> > >       return ControlInfo(indices,
> > >                          ControlValue(static_cast<int32_t>(ctrl.default_value)));
> > >  }
> > > @@ -631,7 +642,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.";
> > 
> > s/registered./registered/
> > 
> > But I would drop this as we already log a message in
> > v4l2MenuControlInfo() (or you could drop the message there instead).
> 
> I added this at the request of Jacopo on the previous version, and I
> agree, because at this point it's an Error - and it catches more error
> paths than just the one highlighted above which would otherwise not
> report a message.

Then I'd drop the first one. If one message bothers me already, two will
be worse :-)

> > > +
> > > +                     continue;
> > > +             }
> > > +
> > > +             ctrls.emplace(controlIds_.back().get(), *info);
> > >       }
> > >  
> > >       controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);
> > > @@ -670,7 +691,7 @@ void V4L2Device::updateControlInfo()
> > >                       continue;
> > >               }
> > >  
> > > -             info = v4l2ControlInfo(ctrl);
> > > +             info = *v4l2ControlInfo(ctrl);
> > >       }
> > >  }
> > >