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

[Laurent Pinchart]

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.

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