[libcamera-devel] [PATCH] libcamera: Fix crash caused by reading uninitialised delayed controls

[David Plowman]

Hi Jacopo

Thanks for the question!

On Wed, 29 Sept 2021 at 10:51, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hello
>
> On Wed, Sep 29, 2021 at 12:58:27AM +0300, Laurent Pinchart wrote:
> > Hi David,
> >
> > Thank you for the patch.
> >
> > On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote:
> > > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74.
> >
> > This should be moved before the Signed-off-by line, with
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=74.
> >
> > > The cause is that we read out delayed values using a frame's sequence
> > > number (DelayedControls::get). But we fill the values up
> > > (DelayedControls::applyControls) incrementing writeCount by only one
> > > even if the sequence number has jumped by several since last
> > > time. This is exactly what happens when frames are being dropped.
> > >
> > > So the fix is to increment writeCount by "as much as the sequence
> > > number has jumped since last time", which means that we just follow
> > > the sequence number directly.
> > >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  src/libcamera/delayed_controls.cpp | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> > > index 90ce7e0b..9667187e 100644
> > > --- a/src/libcamera/delayed_controls.cpp
> > > +++ b/src/libcamera/delayed_controls.cpp
> > > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence)
> > >             }
> > >     }
> > >
> > > -   writeCount_++;
> > > +   writeCount_ = sequence - firstSequence_ + 1;
> >
> > I'm always confused when I read this file, I think one day I'll dive
> > deep and do something about it :-) Until then, this looks good to me.
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > A second pair of eyes would be nice (and testing on IPU3 too).
>
> I've run a few CTS runs and it doesn't seem to introduce regressions.
>
> However I'm wondering how this chaneg plays with queueCount
>
> >
> > >
> > >     while (writeCount_ > queueCount_) {
>                ^
>                this
>
> What I wonder about is: if a frame is dropped, should we advance the
> count and 'burn' the controls that were meant for it ? Because in this
> case it seems to me that if the number of dropped frames exceeds the
> number of controls available in the queue, we end up pushing {} but we
> advance queueCount by one only.

That loop will run, incrementing queueCount (in "push") by 1 each
time, until it has caught up with writeCount - which I believe to be
the correct behaviour. Each call will copy the previous control
entries to the next one, so anyone who asks for values for a frame we
dropped will simply get the most recent values that we did see.

Even if the number of dropped frames exceeds the length of the queue
the behaviour seems fine - the entire queue will fill up with the last
seen values.

>
> If it's intended to burn all the controls meant for dropped frames we
> should probably increment queueCount by the same amount writeCount_
> has been incremented with.

Not sure what you mean by "burn" the controls? The client code can ask
for values from sequence numbers that were never actually seen (if it
wants), and will get the last known values back. Does that make sense?

Best regards
David

>
>
> > >             LOG(DelayedControls, Debug)
>
> >
> > --
> > Regards,
> >
> > Laurent Pinchart