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

[Jacopo Mondi]

Hi David

On Wed, Sep 29, 2021 at 11:17:23AM +0100, David Plowman wrote:
> 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?

Let's assume that we have 3 controls in the values_ queue waiting to
be applied to the 'next' frames

writeCount = x
queueCount = x + 3
values_ = [ C(x+1), C(x+2), C(x+3) ]

We drop 4 frames hence

writeCount = x + 4

and we have to advance queueCount by pushing {} and copying the last
values

values_ = [ C(x+1), C(x+2), C(x+3), C(x+3) ]

What I meant by burnt is that if I'm not mistaken C(x+1) and C(x+2)
are lost and I wonder if we shouldn't instead

values_ = [ C(x+1), C(x+2), C(x+3), C(x+1), C(x+2), C(x+3) ]

when frames are dropped.

Or have I got it all wrong ?


>
> Best regards
> David
>
> >
> >
> > > >             LOG(DelayedControls, Debug)
> >
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart