| Message ID | 20201117132432.238405-1-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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/ > in order to avoid raising abort and crash. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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())... With that fixed, Reviewed-by: Kieran Bingham <kieran.bingham@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. -- Kieran > if (delays_.find(id) == delays_.end()) > return false; >
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@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@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 :-) > -- > Kieran > > >> if (delays_.find(id) == delays_.end()) >> return false; >> >
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@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@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; > >> > >
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); if (delays_.find(id) == delays_.end()) return false;
When pushing a controlone should check if it exists before accessing it, in order to avoid raising abort and crash. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/libcamera/delayed_controls.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)