[libcamera-devel] [PATCH] libcamera: Avoid accessing a non existent control

[Niklas Söderlund]

Hi Jean-Michel,

Thanks for your patch.

On 2020-11-17 23:08:59 +0100, Jean-Michel Hautbois wrote:
> Hi Kieran,
> 
> On 17/11/2020 14:29, Kieran Bingham wrote:
> > Hi Jean-Michel,
> > 
> > Thanks for highlighting this issue, and providing a fix.
> > 
> > 
> > On 17/11/2020 13:24, Jean-Michel Hautbois wrote:
> >> When pushing a controlone should check if it exists before accessing it,
> > 
> > s/controlone/control, one/
> > 
> > s/it,/it/
> 
> Sure... :-)
> 
> >> in order to avoid raising abort and crash.
> >>
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >> ---
> >>  src/libcamera/delayed_controls.cpp | 6 +++++-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> >> index f1c5f890..422deee5 100644
> >> --- a/src/libcamera/delayed_controls.cpp
> >> +++ b/src/libcamera/delayed_controls.cpp
> >> @@ -147,7 +147,11 @@ bool DelayedControls::queue(const ControlList &controls)
> >>  
> >>  	/* Update with new controls. */
> >>  	for (const auto &control : controls) {
> >> -		const ControlId *id = device_->controls().idmap().at(control.first);
> >> +		const ControlIdMap &idmap = device_->controls().idmap();
> >> +		if (idmap.find(control.first) == idmap.end())
> >> +			return false;
> >> +
> >> +		const ControlId *id = idmap.at(control.first);
> >>  
> > 
> > This does two look ups, (idmap.find(), idmap.at()).
> > 
> > We could combine those to a single lookup by first doing the find, and
> > keeping the iterator, then using it (if it is not equal to idmap.end())...
> 
> Yes, indeed !
> 
> > 
> > With that fixed,
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard>
> > 
> > However, given that this patch is against an out of tree patch - I
> > anticipate the fix should be squashed into the relevant patch before
> > integration, so I'll leave this up to Niklas.
> 
> I agree :-)

I have integrated this in the out-of-tree patch as,

diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
index f1c5f890957ec040..4651e71aa53b884a 100644
--- a/src/libcamera/delayed_controls.cpp
+++ b/src/libcamera/delayed_controls.cpp
@@ -146,8 +146,13 @@ bool DelayedControls::queue(const ControlList &controls)
        }
 
        /* Update with new controls. */
+       const ControlIdMap &idmap = device_->controls().idmap();
        for (const auto &control : controls) {
-               const ControlId *id = device_->controls().idmap().at(control.first);
+               const auto &itCtrl = idmap.find(control.first);
+               if (itCtrl == idmap.end())
+                       return false;
+
+               const ControlId *id = itCtrl->second;
 
                if (delays_.find(id) == delays_.end())
                        return false;

> > --
> > Kieran
> > 
> > 
> >>  		if (delays_.find(id) == delays_.end())
> >>  			return false;
> >>
> > 

-- 
Regards,
Niklas Söderlund