[libcamera-devel] [PATCH v2] libcamera: media_device: prevent sign extension on casts
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Feb 18 18:38:27 CET 2020
Hi Laurent,
On 18/02/2020 17:33, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Tue, Feb 18, 2020 at 05:07:41PM +0000, Kieran Bingham wrote:
>> On 18/02/2020 15:17, Laurent Pinchart wrote:
>>> 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 :-)
>>
>> Agh, I indented the left hand items by 4 to indent the whole code block
>> for the commit message.
>>
>> Indenting the whole lot by a tab fits, but it looks a bit weird and too
>> far to the right ... but I don't think it's important.
>
> No big deal, no. I was however referring to lines starting with
> "uint64_t uint64" and "return" that seem to have a different combination
> of spaces and tabs than the other ones.
>
>>>> When compiled with g++ produces the following unexpected output:
>>>
>>> s/with g++/with g++ for a 32-bit platform/
>>
>> No, I don't think that's true.
>> This issue affects a 64-bit platform just the same.
>>
>> If a pointer is passed through here which happens to be
>> 0x00000000f1234567, it will be incorrect when it gets to the kernel.
>>
>> (or any pointer with leading zeros up to a set bit 32...)
>
> Really ? Testing the above code on x86-64 without -m32 prints
>
> ptr 0xf1234567 u64 0xf1234567 uint64 0xf1234567
>
> for me.
Ah, I /may/ have been building the snippit code on the target, when I
thought I had done it on the host... :-D
I.e. I hadn't specified -m32 'explicitly' ... ahem...
So you are correct, this is only an issue when compiled for 32 bit systems.
--
Kieran
>
>>>> 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
--
Kieran
More information about the libcamera-devel
mailing list