[libcamera-devel] [PATCH] libcamera: controls: guard ControlInfoMap against nullptr idmap_

[Laurent Pinchart]

Hi Mattijs,

Thank you for the patch.

On Tue, Apr 04, 2023 at 06:11:16PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
> It's possible to construct a Camera with an unsafe controlInfo_.
> This is the case in the Simple pipeline, where the camera controls are
> not populated.
> 
> With Simple, if we attempt to set a Control, we end up with a segfault
> because the default constructor for ControlInfoMap doesn't
> intialized idmap_ which is initialized at class declaration time as
> 
>   const ControlIdMap *idmap_ = nullptr;
> 
> Add some safeguards in ControlInfoMap to handle this case.
> 
> Link: http://codepad.org/CiLLcPNW
> Link: https://lists.libcamera.org/pipermail/libcamera-devel/2023-April/037439.html
> Suggested-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> Signed-off-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>

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

> ---
> I have build-tested this against master and functionally tested on a
> v0.0.4 android integration branch.
> 
> I've tested that i'm able to do a camera preview even when the
> ScalerCrop control is unavailble.
> 
> Note that Jacopo already discussed alternative implementation by making
> idmap_ an instance instead of a pointer, but that seemed not a good idea
> either:
> 
> https://lists.libcamera.org/pipermail/libcamera-devel/2023-April/037439.html
> ---
>  src/libcamera/controls.cpp | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 6dbf9b348709..b808116c01e5 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -677,6 +677,9 @@ ControlInfoMap::ControlInfoMap(Map &&info, const ControlIdMap &idmap)
>  
>  bool ControlInfoMap::validate()
>  {
> +	if (!idmap_)
> +		return false;
> +
>  	for (const auto &ctrl : *this) {
>  		const ControlId *id = ctrl.first;
>  		auto it = idmap_->find(id->id());
> @@ -719,6 +722,8 @@ bool ControlInfoMap::validate()
>   */
>  ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
>  {
> +	ASSERT(idmap_);
> +
>  	return at(idmap_->at(id));
>  }
>  
> @@ -729,6 +734,8 @@ ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
>   */
>  const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const
>  {
> +	ASSERT(idmap_);
> +
>  	return at(idmap_->at(id));
>  }
>  
> @@ -739,6 +746,9 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const
>   */
>  ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
>  {
> +	if (!idmap_)
> +		return 0;
> +
>  	/*
>  	 * The ControlInfoMap and its idmap have a 1:1 mapping between their
>  	 * entries, we can thus just count the matching entries in idmap to
> @@ -755,6 +765,9 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
>   */
>  ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
>  {
> +	if (!idmap_)
> +		return end();
> +
>  	auto iter = idmap_->find(id);
>  	if (iter == idmap_->end())
>  		return end();
> @@ -770,6 +783,9 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
>   */
>  ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
>  {
> +	if (!idmap_)
> +		return end();
> +
>  	auto iter = idmap_->find(id);
>  	if (iter == idmap_->end())
>  		return end();
> 
> ---
> base-commit: ac7511dc4c594f567ddff27ccc02c30bf6c00bfd
> change-id: 20230404-guard-idmap-1d95f2100aca

-- 
Regards,

Laurent Pinchart