[PATCH] libcamera: delayed_controls: Inherit from Object class

[Laurent Pinchart]

Hi Stanislaw,

On Mon, Feb 24, 2025 at 11:16:45AM +0100, Stanislaw Gruszka wrote:
> On Mon, Feb 24, 2025 at 03:19:34AM +0200, Laurent Pinchart wrote:
> > A second use-after-free bug related to signals staying connected after
> > the receiver DelayedControls instance gets deleted has been found, this
> > time in the simple pipeline handler. Fix the issue once and for all by
> > making the DelayedControls class inherit from Object. This will
> > disconnect signals automatically upon deletion of the receiver.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > Stan, would you be able to test this with the simple pipeline handler ?
> > It should work both with and without your series that deals with the
> > frame start signal, and should fix the crash that Kieran has reported.
> 
> It does not make assertion
> "state_ == ProxyRunning" failed in processStatsThread()
> gone for me. 

Right, that one is addressed by Milan's "PATCH v2 0/5] Fix occasional
software ISP assertion error on stop" series.

This patch should fix a different crash, but now that I wrote this,
Kieran may not have reported it with the simple pipeline handler. He has
reported it for the rkisp1 though.

Would you be able to test this patch on top of Milan's series, without
your changes ? I'd like to make sure it introduces no regression. We can
then apply your series on top.

> I tested this patch alone as well together with
> 
> [PATCH v4 0/2] libcamera: start frame events changes
> [PATCH] pipeline: simple: Create DelayedControls instance once only
> 
> So far, only revert of 9bc8b6a573a63b8c9f5588cdea6da5ae71abd138
> make the assertion gone.
> 
> One interesting thing is the issue is not reproducible when
> killing qcam by sending TERM signal, like for example in
> below script:
> 
> #!/bin/bash -x
> for i in `seq 1 20` ; do
>         ls core &> /dev/null && break;
> 
>         ./git/libcamera/build/src/apps/qcam/qcam &
>         pid=$!
> 
>         sleep `expr $RANDOM % 10`
>         kill $pid
>         sleep `expr $RANDOM % 2`
> done
> 
> I have to manually press X on qcam window to trigger the assertion.
> Usually 2 or 3 times is enough.
> 
> >  include/libcamera/internal/delayed_controls.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
> > index e8d3014d92cb..b64d8bba7cf7 100644
> > --- a/include/libcamera/internal/delayed_controls.h
> > +++ b/include/libcamera/internal/delayed_controls.h
> > @@ -10,13 +10,15 @@
> >  #include <stdint.h>
> >  #include <unordered_map>
> > 
> > +#include <libcamera/base/object.h>
> > +
> >  #include <libcamera/controls.h>
> > 
> >  namespace libcamera {
> > 
> >  class V4L2Device;
> > 
> > -class DelayedControls
> > +class DelayedControls : public Object
> >  {
> >  public:
> >  	struct ControlParams {
> > 
> > base-commit: d476f8358be1536d4edd680c6024f784ff022f5d

-- 
Regards,

Laurent Pinchart