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

[Laurent Pinchart]

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 at 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 at ideasonboard.com>

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

-- 
Regards,

Laurent Pinchart