[libcamera-devel,2/2] android: CameraDevice: Add more camera3_capture_request validation
diff mbox series

Message ID 20210401153123.1217170-2-hiroh@chromium.org
State Superseded
Headers show
Series
  • [libcamera-devel,1/2] android: cameraDevice: Factorize the code of validating camera3_capture_request
Related show

Commit Message

Hirokazu Honda April 1, 2021, 3:31 p.m. UTC 
This adds more validation to camera3_capture_request mainly
about buffer_handle values.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi April 1, 2021, 4:10 p.m. UTC | #1 
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@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 ?

> +		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

Thanks
   j

> +					<< handle->numInts;
> +			return -EINVAL;
> +		}
> +	}
> +
>  	return true;
>  }
>
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Hirokazu Honda April 1, 2021, 4:15 p.m. UTC | #2 
On Fri, Apr 2, 2021 at 1:09 AM Jacopo Mondi <jacopo@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@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
>
> Thanks
>    j
>
> > +                                     << handle->numInts;
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> >       return true;
> >  }
> >
> > --
> > 2.31.0.291.g576ba9dcdaf-goog
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham April 1, 2021, 10:36 p.m. UTC | #3 
Hi Hiro,

On 01/04/2021 17:15, Hirokazu Honda wrote:
> On Fri, Apr 2, 2021 at 1:09 AM Jacopo Mondi <jacopo@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@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@lists.libcamera.org
>>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>

Patch
diff mbox series 

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;
+		}
+
+		const native_handle_t *handle = *outputBuffer.buffer;
+		constexpr int kNativeHandleMaxFds = 1024;
+		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"
+					<< handle->numInts;
+			return -EINVAL;
+		}
+	}
+
 	return true;
 }