[libcamera-devel] [PATCH] android: Protect against null callbacks
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Sep 8 14:21:14 CEST 2020
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 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]]).
I prefer being safe. This is cheap.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>>> CameraDevice *camera = cameraDeviceFromHalId(id);
>>> if (!camera) {
>>> LOG(HAL, Error) << "Invalid camera id '" << id << "'";
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list