| Message ID | 20200908075424.14101-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | de20029a582a71a87d99388a62fb63c86e85028a |
| Headers | show |
| Series |
|
| Related | show |
On Tue, Sep 08, 2020 at 10:54:24AM +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. I forgot to add Reported-by: Coverity CID=298638 > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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; > + } > + > CameraDevice *camera = cameraDeviceFromHalId(id); > if (!camera) { > LOG(HAL, Error) << "Invalid camera id '" << id << "'";
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@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, 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@ideasonboard.com> First of all, Reviewed-by: Umang Jain <email@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. > CameraDevice *camera = cameraDeviceFromHalId(id); > if (!camera) { > LOG(HAL, Error) << "Invalid camera id '" << id << "'";
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@ideasonboard.com> > > First of all, > > Reviewed-by: Umang Jain <email@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 << "'";
Hi Laurent, On 08/09/2020 12:16, Laurent Pinchart wrote: > 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@ideasonboard.com> >> >> First of all, >> >> Reviewed-by: Umang Jain <email@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]]). I prefer being safe. This is cheap. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> CameraDevice *camera = cameraDeviceFromHalId(id); >>> if (!camera) { >>> LOG(HAL, Error) << "Invalid camera id '" << id << "'"; >
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 << "'";
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@ideasonboard.com> --- src/android/camera3_hal.cpp | 2 +- src/android/camera_hal_manager.cpp | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-)