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

[Laurent Pinchart]

Hi Umang,

On Tue, Sep 08, 2020 at 04:00:17PM +0530, Umang Jain wrote:
> On 9/8/20 1:24 PM, 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>
>
> First of all,
> 
> Reviewed-by: Umang Jain <email at uajain.com>
>
> > ---
> >   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;
> > +	}
> > +
> 
> At any point in time, were you able to trigger a crash to which, as a 
> result, is this patch came into existence? Just curious.

No, I haven't. I was looking at the coverity scans, one of the issues
pointed out was that callbacks_ was not initialized. It's a theoretical
issue as the camera service shouldn't behave incorrectly, but I thought
this one may be worth handling correctly. If anyone disagrees I'm happy
to drop the patch (although I'd then submit a smaller patch to drop the
[[maybe_unused]]).

> >   	CameraDevice *camera = cameraDeviceFromHalId(id);
> >   	if (!camera) {
> >   		LOG(HAL, Error) << "Invalid camera id '" << id << "'";

-- 
Regards,

Laurent Pinchart