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

[Mattijs Korpershoek]

Hi Laurent,

Thank you for your review.

On mer., avril 05, 2023 at 05:49, Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:

> On Wed, Apr 05, 2023 at 05:47:41AM +0300, Laurent Pinchart wrote:
>> 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>
>
> Actually, maybe I ask you to extend the ControlInfoMap unit test to
> catch the issue ? The test should crash without this patch, and pass
> with it. This can be done in a separate patch.

Yes, no problem, I will send a V2 extending the ControlInfoMap unit test
in a separate patch.

>
>> > ---
>> > 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