[libcamera-devel,v2,2/2] android: Check if Stream configurations were generated correctly

14026 diff mbox series

The libcamera Android Camera HAL generates camera configurations for the
StillCapture, Raw and ViewFinder stream roles. But there is only a check
if the configuration generation failed, for the StillCapture stream role.

This could lead to a NULL pointer dereference if a pipeline handler fails
to generate a default configuration for one of the other two stream roles.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v2:
- Fix typos in commit message.

 src/android/camera_capabilities.cpp | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Hi Javier, thank you for the patch.

On Sat, Oct 2, 2021 at 6:16 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> The libcamera Android Camera HAL generates camera configurations for the
> StillCapture, Raw and ViewFinder stream roles. But there is only a check
> if the configuration generation failed, for the StillCapture stream role.
>
> This could lead to a NULL pointer dereference if a pipeline handler fails
> to generate a default configuration for one of the other two stream roles.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
> Changes in v2:
> - Fix typos in commit message.
>
>  src/android/camera_capabilities.cpp | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index 87a6e1c6f26..baeedc11500 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -408,6 +408,11 @@ CameraCapabilities::initializeYUVResolutions(const PixelFormat &pixelFormat,
>         std::vector<Size> supportedResolutions;
>         std::unique_ptr<CameraConfiguration> cameraConfig =
>                 camera_->generateConfiguration({ StreamRole::Viewfinder });
> +       if (!cameraConfig) {
> +               LOG(HAL, Error) << "Failed to get supported YUV resolutions";
> +               return supportedResolutions;
> +       }
> +
>         StreamConfiguration &cfg = cameraConfig->at(0);
>
>         for (const Size &res : resolutions) {
> @@ -431,11 +436,17 @@ CameraCapabilities::initializeYUVResolutions(const PixelFormat &pixelFormat,
>  std::vector<Size>
>  CameraCapabilities::initializeRawResolutions(const PixelFormat &pixelFormat)
>  {
> +       std::vector<Size> supportedResolutions;
>         std::unique_ptr<CameraConfiguration> cameraConfig =
>                 camera_->generateConfiguration({ StreamRole::Raw });
> +       if (!cameraConfig) {
> +               LOG(HAL, Error) << "Failed to get supported Raw resolutions";
> +               return supportedResolutions;

return {} may work?
So declaring supportedResolution above is not necessary.

With the nit,
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

> +       }
> +
>         StreamConfiguration &cfg = cameraConfig->at(0);
>         const StreamFormats &formats = cfg.formats();
> -       std::vector<Size> supportedResolutions = formats.sizes(pixelFormat);
> +       supportedResolutions = formats.sizes(pixelFormat);
>
>         return supportedResolutions;
>  }
> --
> 2.31.1
>

Hi Javier,

On Fri, Oct 01, 2021 at 11:15:25PM +0200, Javier Martinez Canillas wrote:
> The libcamera Android Camera HAL generates camera configurations for the
> StillCapture, Raw and ViewFinder stream roles. But there is only a check
> if the configuration generation failed, for the StillCapture stream role.
>
> This could lead to a NULL pointer dereference if a pipeline handler fails
> to generate a default configuration for one of the other two stream roles.

I don't think that's strictly necessary.

The path towards which we get to genreate a config for a role is:

        for (androidFormat : AndroidRequiredFormats) {

                PixelFormat mappedFormat;
                for (libcameraFormat : androidFormat.compatibleFmts) {
                        cameraConfig.pixelFormat = libcameraFormat;

                        status = cameraConfig.validate()l
                        if (status == ok) {
                                mappedFormat = libameraFormat;
                                break;
                        }
                }

                if (!mappedFormat.valid())
                        continue;

                if (mappedFormat == RAW)
                        initializeRawResolutions()

                if (mappedFormat == YUV || mappedFormat == RGB)
                        initializeYUVResolutions()
        }

Hence, if we get to generate a config for a role we know there's a
supported compatible format.

However, as the association between roles and supported pixel formats
is a little fuzzy in pipeline handlers, I guess the better safe than
sorry approach is the correct one

>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

> ---
>
> Changes in v2:
> - Fix typos in commit message.
>
>  src/android/camera_capabilities.cpp | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index 87a6e1c6f26..baeedc11500 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -408,6 +408,11 @@ CameraCapabilities::initializeYUVResolutions(const PixelFormat &pixelFormat,
>  	std::vector<Size> supportedResolutions;
>  	std::unique_ptr<CameraConfiguration> cameraConfig =
>  		camera_->generateConfiguration({ StreamRole::Viewfinder });
> +	if (!cameraConfig) {
> +		LOG(HAL, Error) << "Failed to get supported YUV resolutions";
> +		return supportedResolutions;
> +	}
> +
>  	StreamConfiguration &cfg = cameraConfig->at(0);
>
>  	for (const Size &res : resolutions) {
> @@ -431,11 +436,17 @@ CameraCapabilities::initializeYUVResolutions(const PixelFormat &pixelFormat,
>  std::vector<Size>
>  CameraCapabilities::initializeRawResolutions(const PixelFormat &pixelFormat)
>  {
> +	std::vector<Size> supportedResolutions;
>  	std::unique_ptr<CameraConfiguration> cameraConfig =
>  		camera_->generateConfiguration({ StreamRole::Raw });
> +	if (!cameraConfig) {
> +		LOG(HAL, Error) << "Failed to get supported Raw resolutions";
> +		return supportedResolutions;
> +	}
> +
>  	StreamConfiguration &cfg = cameraConfig->at(0);
>  	const StreamFormats &formats = cfg.formats();
> -	std::vector<Size> supportedResolutions = formats.sizes(pixelFormat);
> +	supportedResolutions = formats.sizes(pixelFormat);

Hiro's right, not required, but I prefer your way as it makes
initializeRawResolutions more similar to initializeYUVResolutions

Up to you!

Thanks
  j


>
>  	return supportedResolutions;
>  }
> --
> 2.31.1
>

Thanks Hiro and Jacobo for the feedback.

On 10/4/21 10:41, Jacopo Mondi wrote:
> Hi Javier,

[snip]

> 
> Hence, if we get to generate a config for a role we know there's a
> supported compatible format.
>

Agreed, that's why I used "could lead to..." (I first wrote "will lead")
but the function should not attempt to dereference a pointer that may be
NULL and rely on the caller to only call it when is safe.

[snip]

>> +
>>  	StreamConfiguration &cfg = cameraConfig->at(0);
>>  	const StreamFormats &formats = cfg.formats();
>> -	std::vector<Size> supportedResolutions = formats.sizes(pixelFormat);
>> +	supportedResolutions = formats.sizes(pixelFormat);
> 
> Hiro's right, not required, but I prefer your way as it makes
> initializeRawResolutions more similar to initializeYUVResolutions
>

Yes, I did if for consistency between the two functions. I don't have a
strong opinion really, I'm happy to change if you believe that's better.

Best regards,