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

Message ID 20221123134232.1937002-1-kieran.bingham@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel,v3] libcamera: v4l2_device: Workaround faulty control menus
Related show

Commit Message

Kieran Bingham Nov. 23, 2022, 1:42 p.m. UTC 
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>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/internal/v4l2_device.h |  4 ++--
 src/libcamera/v4l2_device.cpp            | 23 ++++++++++++++++++-----
 2 files changed, 20 insertions(+), 7 deletions(-)

Comments

Jacopo Mondi Nov. 24, 2022, 12:31 p.m. UTC | #1 
Hi Kieran

   this seems  a better fix indeed: not registering the
control at all if faulty. Users might be wonder why the control is not
exposed, but the warning message should warn them.. speaking of which


On Wed, Nov 23, 2022 at 01:42:32PM +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>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_device.h |  4 ++--
>  src/libcamera/v4l2_device.cpp            | 23 ++++++++++++++++++-----
>  2 files changed, 20 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..c2e21bdf33b7 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.
> +	 * 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";

I would explicitely say "Cannot register control..."

Apart from that
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> +		return std::nullopt;
> +	}
> +
>  	return ControlInfo(indices,
>  			   ControlValue(static_cast<int32_t>(ctrl.default_value)));
>  }
> @@ -631,7 +642,9 @@ 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)
> +			ctrls.emplace(controlIds_.back().get(), *info);
>  	}
>
>  	controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);
> @@ -670,7 +683,7 @@ void V4L2Device::updateControlInfo()
>  			continue;
>  		}
>
> -		info = v4l2ControlInfo(ctrl);
> +		info = *v4l2ControlInfo(ctrl);
>  	}
>  }
>
> --
> 2.34.1
>
Kieran Bingham Nov. 24, 2022, 12:41 p.m. UTC | #2 
On 24/11/2022 12:31, Jacopo Mondi wrote:
> Hi Kieran
> 
>     this seems  a better fix indeed: not registering the
> control at all if faulty. Users might be wonder why the control is not
> exposed, but the warning message should warn them.. speaking of which
> 
> 
> On Wed, Nov 23, 2022 at 01:42:32PM +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>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>   include/libcamera/internal/v4l2_device.h |  4 ++--
>>   src/libcamera/v4l2_device.cpp            | 23 ++++++++++++++++++-----
>>   2 files changed, 20 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..c2e21bdf33b7 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.
>> +	 * 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";
> 
> I would explicitely say "Cannot register control..."
> 

As there are two error paths here that could fail to add a control, I'll 
update the higher layer as follows:


                 std::optional<ControlInfo> info = v4l2ControlInfo(ctrl);

                 if (!info) {
                         LOG(V4L2, Error)
                                 << "Control " << ctrl.name
                                 << " cannot be registered."

                         continue;
                 }

                 ctrls.emplace(controlIds_.back().get(), *info);


> Apart from that
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks

> 
> Thanks
>    j
> 
>> +		return std::nullopt;
>> +	}
>> +
>>   	return ControlInfo(indices,
>>   			   ControlValue(static_cast<int32_t>(ctrl.default_value)));
>>   }
>> @@ -631,7 +642,9 @@ 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)
>> +			ctrls.emplace(controlIds_.back().get(), *info);
>>   	}
>>
>>   	controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);
>> @@ -670,7 +683,7 @@ void V4L2Device::updateControlInfo()
>>   			continue;
>>   		}
>>
>> -		info = v4l2ControlInfo(ctrl);
>> +		info = *v4l2ControlInfo(ctrl);
>>   	}
>>   }
>>
>> --
>> 2.34.1
>>

Patch
diff mbox series 

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..c2e21bdf33b7 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.
+	 * 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";
+		return std::nullopt;
+	}
+
 	return ControlInfo(indices,
 			   ControlValue(static_cast<int32_t>(ctrl.default_value)));
 }
@@ -631,7 +642,9 @@  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)
+			ctrls.emplace(controlIds_.back().get(), *info);
 	}
 
 	controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);
@@ -670,7 +683,7 @@  void V4L2Device::updateControlInfo()
 			continue;
 		}
 
-		info = v4l2ControlInfo(ctrl);
+		info = *v4l2ControlInfo(ctrl);
 	}
 }