libcamera: base: utils: Prevent hex signed extension
diff mbox series

Message ID 20251028182936.14362-1-kieran.bingham@ideasonboard.com
State New
Headers show
Series
  • libcamera: base: utils: Prevent hex signed extension
Related show

Commit Message

Kieran Bingham Oct. 28, 2025, 6:29 p.m. UTC 
When converting signed values to hex strings using the utils::hex
helpers, signed negative values get sign extended to the larger 64 bit
type.

This causes values such as 0x80 to be printed as 0xffffffffffffff80
instead.

Resolve this by first converting all signed integer types to an unsigned
type of the same size before extending to the larger type.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/base/utils.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Kieran Bingham Oct. 28, 2025, 6:30 p.m. UTC | #1 
Quoting Kieran Bingham (2025-10-28 18:29:36)
> When converting signed values to hex strings using the utils::hex
> helpers, signed negative values get sign extended to the larger 64 bit
> type.
> 
> This causes values such as 0x80 to be printed as 0xffffffffffffff80
> instead.
> 
> Resolve this by first converting all signed integer types to an unsigned
> type of the same size before extending to the larger type.
> 

Possibly for posterity and to highlight in release notes this could be:

Fixes: 45bd1f20f6a9 ("libcamera: base: utils: Implement hex() for 8-bit and 16-bit values")

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/base/utils.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> index cb8caaa9bacc..fdc3fd91f1b6 100644
> --- a/include/libcamera/base/utils.h
> +++ b/include/libcamera/base/utils.h
> @@ -95,7 +95,7 @@ _hex hex(T value, unsigned int width = 0);
>  template<>
>  inline _hex hex<int8_t>(int8_t value, unsigned int width)
>  {
> -       return { static_cast<uint64_t>(value), width ? width : 2 };
> +       return { static_cast<uint64_t>(static_cast<uint8_t>(value)), width ? width : 2 };
>  }
>  
>  template<>
> @@ -107,7 +107,7 @@ inline _hex hex<uint8_t>(uint8_t value, unsigned int width)
>  template<>
>  inline _hex hex<int16_t>(int16_t value, unsigned int width)
>  {
> -       return { static_cast<uint64_t>(value), width ? width : 4 };
> +       return { static_cast<uint64_t>(static_cast<uint16_t>(value)), width ? width : 4 };
>  }
>  
>  template<>
> @@ -119,7 +119,7 @@ inline _hex hex<uint16_t>(uint16_t value, unsigned int width)
>  template<>
>  inline _hex hex<int32_t>(int32_t value, unsigned int width)
>  {
> -       return { static_cast<uint64_t>(value), width ? width : 8 };
> +       return { static_cast<uint64_t>(static_cast<uint32_t>(value)), width ? width : 8 };
>  }
>  
>  template<>
> -- 
> 2.51.0
>
Barnabás Pőcze Oct. 29, 2025, 9:03 a.m. UTC | #2 
Hi

2025. 10. 28. 19:29 keltezéssel, Kieran Bingham írta:
> When converting signed values to hex strings using the utils::hex
> helpers, signed negative values get sign extended to the larger 64 bit
> type.
> 
> This causes values such as 0x80 to be printed as 0xffffffffffffff80
> instead.
> 
> Resolve this by first converting all signed integer types to an unsigned
> type of the same size before extending to the larger type.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   include/libcamera/base/utils.h | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> index cb8caaa9bacc..fdc3fd91f1b6 100644
> --- a/include/libcamera/base/utils.h
> +++ b/include/libcamera/base/utils.h
> @@ -95,7 +95,7 @@ _hex hex(T value, unsigned int width = 0);
>   template<>
>   inline _hex hex<int8_t>(int8_t value, unsigned int width)
>   {
> -	return { static_cast<uint64_t>(value), width ? width : 2 };
> +	return { static_cast<uint64_t>(static_cast<uint8_t>(value)), width ? width : 2 };

I think you can omit the `static_cast<uint64_t>()` here and below.

It seems to me there are also tests, so it should not be too
complicated to check the new behaviour.


Regards,
Barnabás Pőcze

>   }
>   
>   template<>
> @@ -107,7 +107,7 @@ inline _hex hex<uint8_t>(uint8_t value, unsigned int width)
>   template<>
>   inline _hex hex<int16_t>(int16_t value, unsigned int width)
>   {
> -	return { static_cast<uint64_t>(value), width ? width : 4 };
> +	return { static_cast<uint64_t>(static_cast<uint16_t>(value)), width ? width : 4 };
>   }
>   
>   template<>
> @@ -119,7 +119,7 @@ inline _hex hex<uint16_t>(uint16_t value, unsigned int width)
>   template<>
>   inline _hex hex<int32_t>(int32_t value, unsigned int width)
>   {
> -	return { static_cast<uint64_t>(value), width ? width : 8 };
> +	return { static_cast<uint64_t>(static_cast<uint32_t>(value)), width ? width : 8 };
>   }
>   
>   template<>
Laurent Pinchart Oct. 29, 2025, 9:22 a.m. UTC | #3 
On Wed, Oct 29, 2025 at 10:03:55AM +0100, Barnabás Pőcze wrote:
> 2025. 10. 28. 19:29 keltezéssel, Kieran Bingham írta:
> > When converting signed values to hex strings using the utils::hex
> > helpers, signed negative values get sign extended to the larger 64 bit
> > type.
> > 
> > This causes values such as 0x80 to be printed as 0xffffffffffffff80
> > instead.
> > 
> > Resolve this by first converting all signed integer types to an unsigned
> > type of the same size before extending to the larger type.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >   include/libcamera/base/utils.h | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> > index cb8caaa9bacc..fdc3fd91f1b6 100644
> > --- a/include/libcamera/base/utils.h
> > +++ b/include/libcamera/base/utils.h
> > @@ -95,7 +95,7 @@ _hex hex(T value, unsigned int width = 0);
> >   template<>
> >   inline _hex hex<int8_t>(int8_t value, unsigned int width)
> >   {
> > -	return { static_cast<uint64_t>(value), width ? width : 2 };
> > +	return { static_cast<uint64_t>(static_cast<uint8_t>(value)), width ? width : 2 };
> 
> I think you can omit the `static_cast<uint64_t>()` here and below.

It looks we could simplify the implementation with the following
(untested) code that replaces all specializations.

template<typename T,
	 std::enable_if_t<std::is_integral<T>::value &&
			  !std::is_same_v<T, bool>> * = nullptr>
_hex hex(T value, unsigned int width = 0)
{
	return {
		static_cast<std::make_unsigned_t<T>>(value),
		width ? width : static_cast<unsigned int>(sizeof(T)) * 2
	};
}

> It seems to me there are also tests, so it should not be too
> complicated to check the new behaviour.
> 
> >   }
> >   
> >   template<>
> > @@ -107,7 +107,7 @@ inline _hex hex<uint8_t>(uint8_t value, unsigned int width)
> >   template<>
> >   inline _hex hex<int16_t>(int16_t value, unsigned int width)
> >   {
> > -	return { static_cast<uint64_t>(value), width ? width : 4 };
> > +	return { static_cast<uint64_t>(static_cast<uint16_t>(value)), width ? width : 4 };
> >   }
> >   
> >   template<>
> > @@ -119,7 +119,7 @@ inline _hex hex<uint16_t>(uint16_t value, unsigned int width)
> >   template<>
> >   inline _hex hex<int32_t>(int32_t value, unsigned int width)
> >   {
> > -	return { static_cast<uint64_t>(value), width ? width : 8 };
> > +	return { static_cast<uint64_t>(static_cast<uint32_t>(value)), width ? width : 8 };
> >   }
> >   
> >   template<>
Barnabás Pőcze Oct. 29, 2025, 9:28 a.m. UTC | #4 
2025. 10. 29. 10:22 keltezéssel, Laurent Pinchart írta:
> On Wed, Oct 29, 2025 at 10:03:55AM +0100, Barnabás Pőcze wrote:
>> 2025. 10. 28. 19:29 keltezéssel, Kieran Bingham írta:
>>> When converting signed values to hex strings using the utils::hex
>>> helpers, signed negative values get sign extended to the larger 64 bit
>>> type.
>>>
>>> This causes values such as 0x80 to be printed as 0xffffffffffffff80
>>> instead.
>>>
>>> Resolve this by first converting all signed integer types to an unsigned
>>> type of the same size before extending to the larger type.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> ---
>>>    include/libcamera/base/utils.h | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
>>> index cb8caaa9bacc..fdc3fd91f1b6 100644
>>> --- a/include/libcamera/base/utils.h
>>> +++ b/include/libcamera/base/utils.h
>>> @@ -95,7 +95,7 @@ _hex hex(T value, unsigned int width = 0);
>>>    template<>
>>>    inline _hex hex<int8_t>(int8_t value, unsigned int width)
>>>    {
>>> -	return { static_cast<uint64_t>(value), width ? width : 2 };
>>> +	return { static_cast<uint64_t>(static_cast<uint8_t>(value)), width ? width : 2 };
>>
>> I think you can omit the `static_cast<uint64_t>()` here and below.
> 
> It looks we could simplify the implementation with the following
> (untested) code that replaces all specializations.
> 
> template<typename T,
> 	 std::enable_if_t<std::is_integral<T>::value &&
> 			  !std::is_same_v<T, bool>> * = nullptr>
> _hex hex(T value, unsigned int width = 0)
> {
> 	return {
> 		static_cast<std::make_unsigned_t<T>>(value),
> 		width ? width : static_cast<unsigned int>(sizeof(T)) * 2
> 	};
> }

Yes, I agree. Or even more:

struct hex {
   uint64_t v;
   unsigned int w;

   template<typename T, std::enable_if_t...>
   hex(T value, unsigned int width = 0)
     : v(static_cast<std::make_unsigned_t<T>>(value)),
       w(width ? width : sizeof(T) * 2)
   { }
};

and then one does not need a separate function at all.


> 
>> It seems to me there are also tests, so it should not be too
>> complicated to check the new behaviour.
>>
>>>    }
>>>    
>>>    template<>
>>> @@ -107,7 +107,7 @@ inline _hex hex<uint8_t>(uint8_t value, unsigned int width)
>>>    template<>
>>>    inline _hex hex<int16_t>(int16_t value, unsigned int width)
>>>    {
>>> -	return { static_cast<uint64_t>(value), width ? width : 4 };
>>> +	return { static_cast<uint64_t>(static_cast<uint16_t>(value)), width ? width : 4 };
>>>    }
>>>    
>>>    template<>
>>> @@ -119,7 +119,7 @@ inline _hex hex<uint16_t>(uint16_t value, unsigned int width)
>>>    template<>
>>>    inline _hex hex<int32_t>(int32_t value, unsigned int width)
>>>    {
>>> -	return { static_cast<uint64_t>(value), width ? width : 8 };
>>> +	return { static_cast<uint64_t>(static_cast<uint32_t>(value)), width ? width : 8 };
>>>    }
>>>    
>>>    template<>
>
Kieran Bingham Oct. 29, 2025, 9:58 a.m. UTC | #5 
Quoting Barnabás Pőcze (2025-10-29 09:28:18)
> 2025. 10. 29. 10:22 keltezéssel, Laurent Pinchart írta:
> > On Wed, Oct 29, 2025 at 10:03:55AM +0100, Barnabás Pőcze wrote:
> >> 2025. 10. 28. 19:29 keltezéssel, Kieran Bingham írta:
> >>> When converting signed values to hex strings using the utils::hex
> >>> helpers, signed negative values get sign extended to the larger 64 bit
> >>> type.
> >>>
> >>> This causes values such as 0x80 to be printed as 0xffffffffffffff80
> >>> instead.
> >>>
> >>> Resolve this by first converting all signed integer types to an unsigned
> >>> type of the same size before extending to the larger type.
> >>>
> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>> ---
> >>>    include/libcamera/base/utils.h | 6 +++---
> >>>    1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> >>> index cb8caaa9bacc..fdc3fd91f1b6 100644
> >>> --- a/include/libcamera/base/utils.h
> >>> +++ b/include/libcamera/base/utils.h
> >>> @@ -95,7 +95,7 @@ _hex hex(T value, unsigned int width = 0);
> >>>    template<>
> >>>    inline _hex hex<int8_t>(int8_t value, unsigned int width)
> >>>    {
> >>> -   return { static_cast<uint64_t>(value), width ? width : 2 };
> >>> +   return { static_cast<uint64_t>(static_cast<uint8_t>(value)), width ? width : 2 };
> >>
> >> I think you can omit the `static_cast<uint64_t>()` here and below.
> > 
> > It looks we could simplify the implementation with the following
> > (untested) code that replaces all specializations.
> > 
> > template<typename T,
> >        std::enable_if_t<std::is_integral<T>::value &&
> >                         !std::is_same_v<T, bool>> * = nullptr>
> > _hex hex(T value, unsigned int width = 0)
> > {
> >       return {
> >               static_cast<std::make_unsigned_t<T>>(value),
> >               width ? width : static_cast<unsigned int>(sizeof(T)) * 2
> >       };
> > }
> 
> Yes, I agree. Or even more:
> 
> struct hex {
>    uint64_t v;
>    unsigned int w;
> 
>    template<typename T, std::enable_if_t...>
>    hex(T value, unsigned int width = 0)
>      : v(static_cast<std::make_unsigned_t<T>>(value)),
>        w(width ? width : sizeof(T) * 2)
>    { }
> };
> 
> and then one does not need a separate function at all.

Thanks - parameterising this all out looks good.

I agree on the tests too - I'll add a test case that failed for me - and
then convert.

--
Kieran


> 
> 
> > 
> >> It seems to me there are also tests, so it should not be too
> >> complicated to check the new behaviour.
> >>
> >>>    }
> >>>    
> >>>    template<>
> >>> @@ -107,7 +107,7 @@ inline _hex hex<uint8_t>(uint8_t value, unsigned int width)
> >>>    template<>
> >>>    inline _hex hex<int16_t>(int16_t value, unsigned int width)
> >>>    {
> >>> -   return { static_cast<uint64_t>(value), width ? width : 4 };
> >>> +   return { static_cast<uint64_t>(static_cast<uint16_t>(value)), width ? width : 4 };
> >>>    }
> >>>    
> >>>    template<>
> >>> @@ -119,7 +119,7 @@ inline _hex hex<uint16_t>(uint16_t value, unsigned int width)
> >>>    template<>
> >>>    inline _hex hex<int32_t>(int32_t value, unsigned int width)
> >>>    {
> >>> -   return { static_cast<uint64_t>(value), width ? width : 8 };
> >>> +   return { static_cast<uint64_t>(static_cast<uint32_t>(value)), width ? width : 8 };
> >>>    }
> >>>    
> >>>    template<>
> > 
>

Patch
diff mbox series 

diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
index cb8caaa9bacc..fdc3fd91f1b6 100644
--- a/include/libcamera/base/utils.h
+++ b/include/libcamera/base/utils.h
@@ -95,7 +95,7 @@  _hex hex(T value, unsigned int width = 0);
 template<>
 inline _hex hex<int8_t>(int8_t value, unsigned int width)
 {
-	return { static_cast<uint64_t>(value), width ? width : 2 };
+	return { static_cast<uint64_t>(static_cast<uint8_t>(value)), width ? width : 2 };
 }
 
 template<>
@@ -107,7 +107,7 @@  inline _hex hex<uint8_t>(uint8_t value, unsigned int width)
 template<>
 inline _hex hex<int16_t>(int16_t value, unsigned int width)
 {
-	return { static_cast<uint64_t>(value), width ? width : 4 };
+	return { static_cast<uint64_t>(static_cast<uint16_t>(value)), width ? width : 4 };
 }
 
 template<>
@@ -119,7 +119,7 @@  inline _hex hex<uint16_t>(uint16_t value, unsigned int width)
 template<>
 inline _hex hex<int32_t>(int32_t value, unsigned int width)
 {
-	return { static_cast<uint64_t>(value), width ? width : 8 };
+	return { static_cast<uint64_t>(static_cast<uint32_t>(value)), width ? width : 8 };
 }
 
 template<>