[libcamera-devel] [PATCH] android: camera_device: Fix crash of accessing a missing map element

[Hirokazu Honda]

On Wed, Oct 28, 2020 at 7:36 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> On Wed, Oct 28, 2020 at 10:33:04AM +0000, Kieran Bingham wrote:
> > Hi Hiro-san,
> >
> > On 28/10/2020 08:57, Hirokazu Honda wrote:
> > > std::map::at() searches std::map by the given key. The commit
> > > e1f9fdb8a5bd096faa34d54804afa48d46d59b28 uses it with 0 to intend to
> > > accessing the first element of the map, but actually access the
> > > element whose key is nullptr. This causes the crash because the map
> > > doesn't have the element with nullptr. This fixes the issue by
> > > replacing the std::map::at() operation by std::map::begin().
> > >
> >
> > Ayeee - I'm sorry - It looks like I certainly messed up something there.
> >
> > Before pushing, I ran that series through our unit-tests, but of course
> > that doesn't cover src/android, and I had obviously neglected to further
> > test the android layer. The clue that I should have done so is in the
> > prefix of the patches, so I'm sorry that I missed that.
> >
> >
> > Even the code it came from was correct:
> >
> > -       FrameBuffer *buffer = buffers.begin()->second;
> > -       resultMetadata = getResultMetadata(descriptor->frameNumber_,
> > -                                          buffer->metadata().timestamp);
> > +       uint64_t timestamp = buffers.at(0)->metadata().timestamp;
> > +       resultMetadata = getResultMetadata(descriptor->frameNumber_,
> > timestamp);
> >
> > So your fix is certainly better.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> >
> > It makes me wonder if the integration processes needs to be able to
> > cover more use-cases, but it's not going to be easy to automate testing
> > of the android layer from a normal build currently.
> >
> > We'd have to mock up the Android HAL calls ... hrm. .. not impossible,
> > but I wonder what the best way to do all that would be (without
> > requiring a full CTS).
>
> We could have basic smoketests, but we're missing the infrastructure to
> run them automatically on a real (or even virtual) device at the moment.
> Clearly something we'll have to address.
>

I honestly haven't run the unit tests. Where are unit tests for
Android HAL adaptation layer?
I test with the ChromeOS camera stack + libcamera.
+1 to adding smoketests.
A HAL client of doing a simple (still) capture + mock/fake libcamera
classes should be nice to have.

Best Regards,
-Hiro
> > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> And we really need to move the timestamp from the buffers to the
> request.
>
> > > ---
> > >  src/android/camera_device.cpp | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index ca60f51..ead8a43 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -1525,7 +1525,7 @@ void CameraDevice::requestComplete(Request *request)
> > >      * It might be appropriate to return a 'correct' (as determined by
> > >      * pipeline handlers) timestamp in the Request itself.
> > >      */
> > > -   uint64_t timestamp = buffers.at(0)->metadata().timestamp;
> > > +   uint64_t timestamp = buffers.begin()->second->metadata().timestamp;
> > >     resultMetadata = getResultMetadata(descriptor->frameNumber_, timestamp);
> > >
> > >     /* Handle any JPEG compression. */
>
> --
> Regards,
>
> Laurent Pinchart