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

[Laurent Pinchart]

Hi Hiro-san,

On Thu, Oct 29, 2020 at 04:01:14PM +0900, Hirokazu Honda wrote:
> On Wed, Oct 28, 2020 at 7:36 PM Laurent Pinchart wrote:
> > On Wed, Oct 28, 2020 at 10:33:04AM +0000, Kieran Bingham wrote:
> > > 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.

We don't have custom unit tests for the camera HAL, we also rely on the
Chrome OS camera tests, and CTS. We however have no infrastructure to
automate this at the moment, so it's all manual, and doesn't get tested
on every commit :-S That's something I would really like to fix, but of
course we can't pause development for a month to put all the focus on
test automation :-)

> +1 to adding smoketests.
> A HAL client of doing a simple (still) capture + mock/fake libcamera
> classes should be nice to have.
> 
> > > > 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