[libcamera-devel] [PATCH] pipeline: raspberrypi: Fix possible null dereference

[Naushir Patuck]

Hi Kieran,

Thank you for the review!

On Mon, 23 May 2022 at 08:31, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Quoting Laurent Pinchart via libcamera-devel (2022-05-22 18:34:15)
> > Hi Naush,
> >
> > Thank you for the patch.
> >
> > On Fri, May 20, 2022 at 01:49:19PM +0100, Naushir Patuck via
> libcamera-devel wrote:
> > > The freeBuffers() cleanup code calls into the IPA to unmap and free
> shared
> > > buffers. However, this function could be called before the IPA has
> opened (via
> > > registerCamera()), causing a segmentation fault. Fix this by guarding
> against
> > > calling the IPA if it has not been opened.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 2636acb758b7..26cd4e5f2b99 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1484,10 +1484,12 @@ void PipelineHandlerRPi::mapBuffers(Camera
> *camera, const RPi::BufferMap &buffer
> > >
> > >  void RPiCameraData::freeBuffers()
> > >  {
> > > -     /* Copy the buffer ids from the unordered_set to a vector to
> pass to the IPA. */
> > > -     std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(),
> ipaBuffers_.end());
> > > -     ipa_->unmapBuffers(ipaBuffers);
> > > -     ipaBuffers_.clear();
> > > +     if (ipa_) {
> > > +             /* Copy the buffer ids from the unordered_set to a
> vector to pass to the IPA. */
> > > +             std::vector<unsigned int>
> ipaBuffers(ipaBuffers_.begin(), ipaBuffers_.end());
> >
> > I'll wrap this when applying.
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> Can this be handled more generically by the interface layer between the
> IPA? (Doesn't need to be done for this patch), but it seems like
> something that could occur in other places (/pipelines) too.
>

In this particular instance, I don't think so.  The ipa_ variable itself is
uninitialised
at this point, so the IPA interface doesn't even exist yet :)

I'm sure in other cases, errors like this could be handled in the interface
layer.

Regards,
Naush



>
> --
> Kieran
>
> >
> > > +             ipa_->unmapBuffers(ipaBuffers);
> > > +             ipaBuffers_.clear();
> > > +     }
> > >
> > >       for (auto const stream : streams_)
> > >               stream->releaseBuffers();
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220523/786aa9e2/attachment-0001.htm>