[libcamera-devel] [libcamera-devel, v3, 1/1] libcamera: converter: Check converter validity

[Laurent Pinchart]

Hi Suhrid,

Thank you for the patch.

On Thu, Mar 02, 2023 at 11:06:29AM -0800, Suhrid Subramaniam via libcamera-devel wrote:
> - In cases where ConverterFactoryBase::create returns a nullptr,
>   converter_->isValid() causes a segmentation fault.
> 
> - Solve this by checking if converter_ is a nullptr.
> 
> - Additionally, check for converter validity in the create function itself
>   and return a nullptr if the converter is invalid.

The commit message can be improved by avoiding the bullet list:

The ConverterFactoryBase::create() function returns a nullptr when no
converter is found. The only caller, SimpleCameraData::init(), checks if
the converter is valid with isValid(), but doesn't check if the pointer
is null, which can lead to a crash.

We could check both pointer validity and converter validity in the
caller, but to limit the complexity in callers, it is better to check
the converter validity in the create() function and return a null
pointer when no valid converter is found.

> Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam at mediatek.com>
> ---
>  src/libcamera/converter.cpp              | 3 ++-
>  src/libcamera/pipeline/simple/simple.cpp | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index 3de39cff..38141da6 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -226,8 +226,9 @@ std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
>  			<< "Creating converter from "
>  			<< factory->name_ << " factory with "
>  			<< (it == compatibles.end() ? "no" : media->driver()) << " alias.";
> +		std::unique_ptr<Converter> converter_ = factory->createInstance(media);

The variable should be named 'converter' as it's a local variable. We
use trailing underscores for class members only.

>  
> -		return factory->createInstance(media);
> +		return converter_->isValid() ? std::move(converter_) : nullptr;

The std::move() can prevent return value optimization. It would be best
to write

		if (!converter_->isValid())
			return nullptr;

		return converter_;

We could even keep iterating over the other factories:

		if (converter_->isValid())
			return converter;

It may not be very useful at this point, but it wouldn't hurt and could
possibly help later.

If you're fine with these changes, there's no need to submit a new
version, I can apply them locally.

>  	}
>  
>  	return nullptr;
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 24ded4db..2423ec10 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -493,7 +493,7 @@ int SimpleCameraData::init()
>  	MediaDevice *converter = pipe->converter();
>  	if (converter) {
>  		converter_ = ConverterFactoryBase::create(converter);
> -		if (!converter_->isValid()) {
> +		if (!converter_) {
>  			LOG(SimplePipeline, Warning)
>  				<< "Failed to create converter, disabling format conversion";
>  			converter_.reset();

-- 
Regards,

Laurent Pinchart