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

2857 diff mbox series

Storing the pointer address of the topology structures using a cast to
__u64 can fail on 32 bit binaries running on a 64 bit kernel due to sign
extension of the pointer.

Convert the casting to use std::uintptr_t which does not sign-extend the value.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/media_device.cpp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Hi Kieran,

Thank you for the patch.

On Tue, Feb 18, 2020 at 12:51:39PM +0000, Kieran Bingham wrote:
> Storing the pointer address of the topology structures using a cast to
> __u64 can fail on 32 bit binaries running on a 64 bit kernel due to sign
> extension of the pointer.

I really wonder why... I've tested the following code:

        void *ptr = reinterpret_cast<void *>(0xf1234567);
        uint64_t u64 = reinterpret_cast<uint64_t>(ptr);

        std::cout << "ptr " << ptr << " u64 0x" << std::hex << u64 << std::endl;

with both g++ and clang++. They respectively gave me

ptr 0xf1234567 u64 0xfffffffff1234567
ptr 0xf1234567 u64 0xf1234567

I wonder if this is undefined behaviour. The standard says

"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]"

I'd say that gcc is not unsurprising :-)

You may want to make a note about this in the commit message.

> Convert the casting to use std::uintptr_t which does not sign-extend the value.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/media_device.cpp | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index e1ae34f88455..2d493e6c895f 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -231,10 +231,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<std::uintptr_t>(ents);
> +		topology.ptr_interfaces = reinterpret_cast<std::uintptr_t>(interfaces);
> +		topology.ptr_links = reinterpret_cast<std::uintptr_t>(links);
> +		topology.ptr_pads = reinterpret_cast<std::uintptr_t>(pads);

You need to include stdint for std::uintptr_t. As we use the C-style
headers, I recommend stdint.h and uintptr_t.

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

>  
>  		ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
>  		if (ret < 0) {