[libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a static function to check a buffer validness

[Laurent Pinchart]

Hi Hiro,

On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote:
> On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote:
> > On Fri, Apr 16, 2021 at 10:43:33PM +0900, Hirokazu Honda wrote:
> > > On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda wrote:
> > > > On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart wrote:
> > > > > On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:
> > > > > > This adds a static function to CameraBuffer class that checks if
> > > > > > a buffer_handle is valid with a platform dependent framework.
> > > > > > For example, the function validates a buffer using
> > > > > > cros::CameraBufferManager on ChromeOS.
> > > > > >
> > > > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > > > ---
> > > > > >  src/android/camera_buffer.h              |  6 ++++++
> > > > > >  src/android/mm/cros_camera_buffer.cpp    | 18 ++++++++++++++++++
> > > > > >  src/android/mm/generic_camera_buffer.cpp |  9 +++++++++
> > > > > >  3 files changed, 33 insertions(+)
> > > > > >
> > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> > > > > > index 7e8970b4..ade082c3 100644
> > > > > > --- a/src/android/camera_buffer.h
> > > > > > +++ b/src/android/camera_buffer.h
> > > > > > @@ -20,6 +20,8 @@ public:
> > > > > >       CameraBuffer(buffer_handle_t camera3Buffer, int flags);
> > > > > >       ~CameraBuffer();
> > > > > >
> > > > > > +     static bool isValidBuffer(buffer_hanlde_t camera3Buffer);
> > > > > > +
> > > > > >       bool isValid() const;
> > > > > >
> > > > > >       unsigned int numPlanes() const;
> > > > > > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)       \
> > > > > >  CameraBuffer::~CameraBuffer()                                                \
> > > > > >  {                                                                    \
> > > > > >  }                                                                    \
> > > > > > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer)             \
> > > > > > +{                                                                    \
> > > > > > +     return Private::isValidBuffer(buffer)                           \
> > > > > > +}                                                                    \
> > > > > >  bool CameraBuffer::isValid() const                                   \
> > > > > >  {                                                                    \
> > > > > >       const Private *const d = LIBCAMERA_D_PTR();                     \
> > > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> > > > > > index 1a4fd5d1..f8449dfd 100644
> > > > > > --- a/src/android/mm/cros_camera_buffer.cpp
> > > > > > +++ b/src/android/mm/cros_camera_buffer.cpp
> > > > > > @@ -24,6 +24,8 @@ public:
> > > > > >               buffer_handle_t camera3Buffer, int flags);
> > > > > >       ~Private();
> > > > > >
> > > > > > +     static bool isValidBuffer(buffer_handle_t camera3Buffer);
> > > > > > +
> > > > > >       bool isValid() const { return valid_; }
> > > > > >
> > > > > >       unsigned int numPlanes() const;
> > > > > > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
> > > > > >       return bufferManager_->GetPlaneSize(handle_, 0);
> > > > > >  }
> > > > > >
> > > > > > +/* static */
> > > > > > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)
> > > > > > +{
> > > > > > +     cros::CameraBufferManager *bufferManager =
> > > > > > +             cros::CameraBufferManager::GetInstance();
> > > > > > +
> > > > > > +     int ret = bufferManager->Register(camera3Buffer);
> > > > > > +     if (ret) {
> > > > > > +             return false;
> > > > > > +     }
> > > > >
> > > > > No need for braces.
> > > > >
> > > > > > +
> > > > > > +     bufferManager->Deregister(camera3Buffer);
> > > > >
> > > > > This seems quite inefficient, as it will lead to registering all
> > > > > buffers, even the ones we don't need to map to the CPU. For buffers that
> > > > > we need to map to the CPU, we will register and unregister them here,
> > > > > and then register them later.
> > > > >
> > > > > Could we do better than this ?
> > > >
> > > > We may want to add verifyBuffer() to CameraBufferManagerInterface to
> > > > do a cheap verification, basically only do
> > > > camera_buffer_handle_t::FromBufferHandle().
> > > > +Shik Chen +Ricky Liang +Tomasz Figa how do you think?
> > >
> > > Added https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340
> >
> > Could you explain what kind of errors you envision this will catch ?
> > When would the camera HAL implementation receive an incorrect buffer
> > handle in the first place ?
> 
> It checks a magic number of the structure is the correct one.
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_handle.h;l=82;drc=b45faaeee6b4a79906678746a2d619ccea361358
> 
> I have no idea about any situation in the product honestly; our test
> tests that processCaptureRequest() fails when an invalid buffer is
> passed.
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_frame_test.cc;l=1397;drc=34d10525925863b7dddb3db830d45365b2d7e25a

In the normal case the buffer handle should be valid. What I'm wondering
is if there are valid use cases (even if they're rare) where the HAL
could receive an invalid handle, or if that would be the consequence of
a serious bug in the camera service. In the latter case, wouldn't it be
better to add the check in the camera service just before calling
.process_capture_request() ? That would cover all HALs in one go.

> > > > > > +
> > > > > > +     return true;
> > > > > > +}
> > > > > > +
> > > > > >  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> > > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> > > > > > index 929e078a..07a07372 100644
> > > > > > --- a/src/android/mm/generic_camera_buffer.cpp
> > > > > > +++ b/src/android/mm/generic_camera_buffer.cpp
> > > > > > @@ -24,6 +24,8 @@ public:
> > > > > >               buffer_handle_t camera3Buffer, int flags);
> > > > > >       ~Private();
> > > > > >
> > > > > > +     static bool isValidBuffer(buffer_handle_t camera3Buffer);
> > > > > > +
> > > > > >       unsigned int numPlanes() const;
> > > > > >
> > > > > >       Span<uint8_t> plane(unsigned int plane);
> > > > > > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
> > > > > >                                     maxJpegBufferSize);
> > > > > >  }
> > > > > >
> > > > > > +/* static */
> > > > > > +bool CameraBuffer::Private::isValidBuffer(
> > > > > > +     [[maybe_unused]] buffer_handle_t camera3Buffer)
> > > > > > +{
> > > > > > +     return true;
> > > > > > +}
> > > > > > +
> > > > > >  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION

-- 
Regards,

Laurent Pinchart