[1/2] libipa: awb: Fix non-virtual destructor warning in AwbStats

22853 diff mbox series

The AwbStats structure has virtual functions but a publicly accessible
non-virtual destructors. This can cause undefined behaviour if deleting
a derived class instance through a pointer to the base AwbStats class.

The problem is theoretical only as no code in libcamera is expected to
perform such deletion, but compilers can't know that and will emit a
warning if the -Wnon-virtual-dtor option is enabled.

Fixing this can be done by declaring a virtual public destructor in the
AwbStats class. A more efficient alternative is to declare a protected
non-virtual destructor, ensuring that instances can't be deleted through
a pointer to the base class. Do so, and mark the derived RkISP1AwbStats
as final to avoid the same warning.

Fixes: 6f663990a0f7 ("libipa: Add AWB algorithm base class")
Reported-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/libipa/awb.h              | 3 +++
 src/ipa/rkisp1/algorithms/awb.cpp | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

Hi Laurent,

thank you for the fix.

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> The AwbStats structure has virtual functions but a publicly accessible
> non-virtual destructors. This can cause undefined behaviour if deleting
> a derived class instance through a pointer to the base AwbStats class.
>
> The problem is theoretical only as no code in libcamera is expected to
> perform such deletion, but compilers can't know that and will emit a
> warning if the -Wnon-virtual-dtor option is enabled.
>
> Fixing this can be done by declaring a virtual public destructor in the
> AwbStats class. A more efficient alternative is to declare a protected
> non-virtual destructor, ensuring that instances can't be deleted through
> a pointer to the base class. Do so, and mark the derived RkISP1AwbStats
> as final to avoid the same warning.
>
> Fixes: 6f663990a0f7 ("libipa: Add AWB algorithm base class")
> Reported-by: Milan Zamazal <mzamazal@redhat.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I can confirm it builds fine with this in my environment (CentOS 9).

> ---
>  src/ipa/libipa/awb.h              | 3 +++
>  src/ipa/rkisp1/algorithms/awb.cpp | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> index a86581adf43e..4bab7a451ce5 100644
> --- a/src/ipa/libipa/awb.h
> +++ b/src/ipa/libipa/awb.h
> @@ -27,6 +27,9 @@ struct AwbResult {
>  struct AwbStats {
>  	virtual double computeColourError(const RGB<double> &gains) const = 0;
>  	virtual RGB<double> rgbMeans() const = 0;
> +
> +protected:
> +	~AwbStats() = default;
>  };
>  
>  class AwbAlgorithm
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 6f9d454eb88f..eafe93081bb1 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -42,7 +42,7 @@ constexpr int32_t kDefaultColourTemperature = 5000;
>  /* Minimum mean value below which AWB can't operate. */
>  constexpr double kMeanMinThreshold = 2.0;
>  
> -class RkISP1AwbStats : public AwbStats
> +class RkISP1AwbStats final : public AwbStats
>  {
>  public:
>  	RkISP1AwbStats(const RGB<double> &rgbMeans)

Hi Laurent,

Thanks for fixing that.

On Tue, Feb 25, 2025 at 12:04:37AM +0200, Laurent Pinchart wrote:
> The AwbStats structure has virtual functions but a publicly accessible
> non-virtual destructors. This can cause undefined behaviour if deleting
> a derived class instance through a pointer to the base AwbStats class.
> 
> The problem is theoretical only as no code in libcamera is expected to
> perform such deletion, but compilers can't know that and will emit a
> warning if the -Wnon-virtual-dtor option is enabled.
> 
> Fixing this can be done by declaring a virtual public destructor in the
> AwbStats class. A more efficient alternative is to declare a protected
> non-virtual destructor, ensuring that instances can't be deleted through
> a pointer to the base class. Do so, and mark the derived RkISP1AwbStats
> as final to avoid the same warning.
> 
> Fixes: 6f663990a0f7 ("libipa: Add AWB algorithm base class")
> Reported-by: Milan Zamazal <mzamazal@redhat.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Regards,
Stefan

> ---
>  src/ipa/libipa/awb.h              | 3 +++
>  src/ipa/rkisp1/algorithms/awb.cpp | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> index a86581adf43e..4bab7a451ce5 100644
> --- a/src/ipa/libipa/awb.h
> +++ b/src/ipa/libipa/awb.h
> @@ -27,6 +27,9 @@ struct AwbResult {
>  struct AwbStats {
>  	virtual double computeColourError(const RGB<double> &gains) const = 0;
>  	virtual RGB<double> rgbMeans() const = 0;
> +
> +protected:
> +	~AwbStats() = default;
>  };
>  
>  class AwbAlgorithm
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 6f9d454eb88f..eafe93081bb1 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -42,7 +42,7 @@ constexpr int32_t kDefaultColourTemperature = 5000;
>  /* Minimum mean value below which AWB can't operate. */
>  constexpr double kMeanMinThreshold = 2.0;
>  
> -class RkISP1AwbStats : public AwbStats
> +class RkISP1AwbStats final : public AwbStats
>  {
>  public:
>  	RkISP1AwbStats(const RGB<double> &rgbMeans)
> -- 
> Regards,
> 
> Laurent Pinchart
>