[libcamera-devel] [PATCH 2/2] android: CameraDevice: Add more camera3_capture_request validation

[Kieran Bingham]

Hi Hiro,

On 01/04/2021 17:15, Hirokazu Honda wrote:
> On Fri, Apr 2, 2021 at 1:09 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
>>
>> Hi Hiro,
>>
>> On Fri, Apr 02, 2021 at 12:31:23AM +0900, Hirokazu Honda wrote:
>>> This adds more validation to camera3_capture_request mainly
>>> about buffer_handle values.
>>>
>>> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
>>> ---
>>>  src/android/camera_device.cpp | 27 ++++++++++++++++++++++++++-
>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>> index 48eb471d..4ed4d3ff 100644
>>> --- a/src/android/camera_device.cpp
>>> +++ b/src/android/camera_device.cpp
>>> @@ -263,11 +263,36 @@ bool isValidRequest(camera3_capture_request_t *camera3Request)
>>>               return false;
>>>       }
>>>
>>> -     if (!camera3Request->num_output_buffers) {
>>> +     if (!camera3Request->num_output_buffers ||
>>> +         !camera3Request->output_buffers) {
>>>               LOG(HAL, Error) << "No output buffers provided";
>>>               return -EINVAL;
>>>       }
>>>
>>> +     for (uint32_t i = 0; i < camera3Request->num_output_buffers; i++) {
>>> +             const camera3_stream_buffer_t &outputBuffer =
>>> +                     camera3Request->output_buffers[i];
>>> +             if (!outputBuffer.buffer || !(*outputBuffer.buffer)) {
>>> +                     LOG(HAL, Error) << "Invalid native handle";
>>> +                     return -EINVAL;
>>
>> return false ?
>>
>>> +             }
>>> +
>>> +             const native_handle_t *handle = *outputBuffer.buffer;
>>> +             constexpr int kNativeHandleMaxFds = 1024;
>>
>> Where does the 1024 limit comes from ?
>>
> 
> It comes from NATIVE_HANDLE_MAX_FDS and NATIVE_HANDLE_MAX_INTS in
> native_handle.h.
> https://android.googlesource.com/platform/system/core/+/master/libcutils/include/cutils/native_handle.h#26
> Somehow native_handle.h in include/android doesn't have the values..
> 
> -Hiro
> 
>>> +             if (handle->numFds < 0 || handle->numFds > kNativeHandleMaxFds) {
>>> +                     LOG(HAL, Error) << "Invalid number of fds: "
>>> +                                     << handle->numFds;
>>> +                     return -EINVAL;
>>> +             }
>>> +
>>> +             constexpr int kNativeHandleMaxInts = 1024;
>>> +             if (handle->numInts < 0 || handle->numInts > kNativeHandleMaxInts) {
>>> +                     LOG(HAL, Error) << "Invalid number of data"
>>
>> "of data:" for consistency

Don't forget the space after the ':'

"Invalid number of data" doesn't make much sense to me, but I'm not
actually sure what handle->numInts is storing?

How about:

"Unsupported quantity of ints: "

But I'm not even fond of that ;-) - Perhaps it doesn't matter so much as
the error will lead the reader here somehow anyway.

>>
>> Thanks
>>    j
>>
>>> +                                     << handle->numInts;
>>> +                     return -EINVAL;

And of course all the return values need to be boolean.


>>> +             }
>>> +     }
>>> +
>>>       return true;
>>>  }
>>>
>>> --
>>> 2.31.0.291.g576ba9dcdaf-goog
>>>
>>> _______________________________________________
>>> libcamera-devel mailing list
>>> libcamera-devel at lists.libcamera.org
>>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran