[libcamera-devel] [PATCH] android: Protect against null callbacks

[Niklas Söderlund]

Hi Laurent,

Thanks for your patch.

On 2020-09-08 10:54:24 +0300, Laurent Pinchart wrote:
> According to the Android camera HAL C interface documentation, the
> camera service is supposed to set callbacks after initializing the HAL
> and calling get_number_of_cameras(), before any other calls to the
> module. We rely on this behaviour and use callbacks unconditionally,
> which would lead to a crash if the camera service behaved incorrectly.
> 
> While the camera service isn't supposed to behave incorrectly,
> gracefully handling the error when opening cameras isn't costly, and
> provides better diagnostic than a crash.
> 
> While at it, removed an unneeded [[maybe_unused]] attribute.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> ---
>  src/android/camera3_hal.cpp        | 2 +-
>  src/android/camera_hal_manager.cpp | 7 ++++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp
> index 67a497b8c829..d6e04af21470 100644
> --- a/src/android/camera3_hal.cpp
> +++ b/src/android/camera3_hal.cpp
> @@ -32,7 +32,7 @@ static int hal_get_camera_info(int id, struct camera_info *info)
>  	return cameraManager.getCameraInfo(id, info);
>  }
>  
> -static int hal_set_callbacks([[maybe_unused]] const camera_module_callbacks_t *callbacks)
> +static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)
>  {
>  	cameraManager.setCallbacks(callbacks);
>  
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index a1805ebccf24..05b474010b1d 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -29,7 +29,7 @@ LOG_DECLARE_CATEGORY(HAL);
>   */
>  
>  CameraHalManager::CameraHalManager()
> -	: cameraManager_(nullptr), numInternalCameras_(0),
> +	: cameraManager_(nullptr), callbacks_(nullptr), numInternalCameras_(0),
>  	  nextExternalCameraId_(firstExternalCameraId_)
>  {
>  }
> @@ -70,6 +70,11 @@ CameraDevice *CameraHalManager::open(unsigned int id,
>  {
>  	MutexLocker locker(mutex_);
>  
> +	if (!callbacks_) {
> +		LOG(HAL, Error) << "Can't open camera before callbacks are set";
> +		return nullptr;
> +	}
> +
>  	CameraDevice *camera = cameraDeviceFromHalId(id);
>  	if (!camera) {
>  		LOG(HAL, Error) << "Invalid camera id '" << id << "'";
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund