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

[Jacopo Mondi]

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.

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.


> >  		LOG(DelayedControls, Debug)

>
> --
> Regards,
>
> Laurent Pinchart