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

[Kieran Bingham]

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



> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  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. */
> --
> 2.29.0.rc2.309.g374f81d7ae-goog
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran