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

[David Plowman]

Hi Jacopo

Thanks for clarifying!

On Wed, 29 Sept 2021 at 11:30, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> 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) ]

Yes, this is what we get.

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

I wouldn't describe them as "lost". The client code can still access
them with get(x+1), get(x+3) etc.

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

I think the trouble with this is that DelayedControls are more of an
array than a queue. Don't think of it in terms of accessing "the most
recent values" or "the values from n frames ago". We access them with
an array index, which happens to be the actual frame sequence number
(more or less). (The fact that only the 16 most recent items are
stored is kind of an "implementation detail".)

So with those values, the code would, for frame x+4, call get(x+4) and
come back with C(x+1) - not the right values.

>
> when frames are dropped.
>
> Or have I got it all wrong ?

Have I explained it any better?  :)

David

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