| Message ID | 20200218141839.5574-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 90240a79506a3c400f3af4cb0b08746ae87c79e2 |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@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) {
Hi Laurent, On 18/02/2020 15:17, Laurent Pinchart wrote: > 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 :-) 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. > >> >> 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...) >> 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@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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) { >
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. > >> 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@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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) {
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@ideasonboard.com> >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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) { >
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) {
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 << std::endl; return 0; } When compiled with g++ produces the following unexpected output: 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@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(-)