libcamera: delayed_controls: Inherit from Object class

22838 diff mbox series

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

 include/libcamera/internal/delayed_controls.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


base-commit: d476f8358be1536d4edd680c6024f784ff022f5d
--
Regards,

Laurent Pinchart

Hi all,

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

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.

Regards
Stanislaw


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

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

Quoting Laurent Pinchart (2025-02-24 21:24:54)
> 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@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.
> 

Yes, the delayed controls crash I hit was on rkisp1 with a video
multiplexor on the pipeline that lets me connect multiple cameras to a
single ISP
(https://www.arducam.com/product/multi-camera-v2-1-adapter-raspberry-pi/)

When I get chance, I'll also test this patch in the rkisp1 context, but
that may take some time.

--
Kieran

> 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

Hi Laurent,

On Mon, Feb 24, 2025 at 11:24:54PM +0200, Laurent Pinchart wrote:
> 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@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.

Tested on master with Milan's 
[PATCH v3 0/6] Fix occasional software ISP assertion error on stop

"state_ == ProxyRunning" assertion is gone and I did not encounter
any regression with simple pipeline. Things looks good.

Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

Regards
Stanislaw