[libcamera-devel] [PATCH v2] libcamera: media_device: prevent sign extension on casts

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Tue, Feb 18, 2020 at 02:18:39PM +0000, Kieran Bingham wrote:
> The conversion of pointers to integers is implementation defined and
> differs between g++ and clang++ when utilising a uint64_t type.
> 
>     #include <iostream>
> 
>     int main(int argc, char **argv)
>     {
>         void *ptr = reinterpret_cast<void *>(0xf1234567);
>         uint64_t u64 = reinterpret_cast<uint64_t>(ptr);
> 	uint64_t uint64 = reinterpret_cast<uintptr_t>(ptr);
> 
>         std::cout << "ptr " << ptr
> 		  << " u64 0x" << std::hex << u64
> 		  << " uint64 0x" << std::hex << uint64

Maybe " ptr -> u64 0x" and " ptr -> uintptr_t -> u64 0x" ?

> 		  << std::endl;
> 
> 	return 0;
>     }

That's a weird mix of tabs and spaces :-)

> 
> When compiled with g++ produces the following unexpected output:

s/with g++/with g++ for a 32-bit platform/

> 
>   ptr 0xf1234567 u64 0xfffffffff1234567 uint64 0xf1234567
> 
> The standards states:
> 
> "A pointer can be explicitly converted to any integral type large enough
> to hold all values of its type. The mapping function is
> implementation-defined. [Note: It is intended to be unsurprising to
> those who know the addressing structure of the underlying machine. — end
> note]"
> 
> And as such the g++ implementation appears to be little more surprising
> than expected in this situation.
> 
> The MediaDevice passes pointers to the kernel via the struct
> media_v2_topology in which pointers are cast using a uint64 type (__u64),
> which is affected by the sign extension described above when BIT(32) is
> set and causes an invalid address to be given to the kernel.
> 
> Ensure that we cast using uintptr_t which is not affected by the sign
> extension issue.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
> v2:
>  - Expand commit message to explain the underlying issue.
>  - include stdint.h
>  - use uintptr_t instead of std::uintptr_t
> 
>  src/libcamera/media_device.cpp | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index fad475b9ac76..0d6b5efd9e7a 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <errno.h>
>  #include <fcntl.h>
> +#include <stdint.h>
>  #include <string>
>  #include <string.h>
>  #include <sys/ioctl.h>
> @@ -236,10 +237,10 @@ int MediaDevice::populate()
>  	 */
>  	while (true) {
>  		topology.topology_version = 0;
> -		topology.ptr_entities = reinterpret_cast<__u64>(ents);
> -		topology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);
> -		topology.ptr_links = reinterpret_cast<__u64>(links);
> -		topology.ptr_pads = reinterpret_cast<__u64>(pads);
> +		topology.ptr_entities = reinterpret_cast<uintptr_t>(ents);
> +		topology.ptr_interfaces = reinterpret_cast<uintptr_t>(interfaces);
> +		topology.ptr_links = reinterpret_cast<uintptr_t>(links);
> +		topology.ptr_pads = reinterpret_cast<uintptr_t>(pads);
>  
>  		ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
>  		if (ret < 0) {

-- 
Regards,

Laurent Pinchart