[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_ro
From: |
Laszlo Ersek |
Subject: |
Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read |
Date: |
Wed, 11 Mar 2020 11:28:26 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
On 03/11/20 08:08, Markus Armbruster wrote:
> Alex Williamson <address@hidden> writes:
>
>> On Wed, 11 Mar 2020 00:14:31 +0100
>> Laszlo Ersek <address@hidden> wrote:
> [...]
>>> So from a memcpy() and range perspective, the patch looks OK. But
>>> there's still a wart I dislike: we should never perform pointer
>>> arithmetic on a (void*). I suggest casting (vdev->rom) to (uint8_t*) or
>>> (unsigned char*) first.
>>>
>>> Here's an excerpt from the ISO C99 standard:
>>>
>>> -v-
>>> 6.5.6 Additive operators
>>>
>>> Constraints
>>>
>>> 2 For addition, either both operands shall have arithmetic type, or one
>>> operand shall be a pointer to an object type and the other shall have
>>> integer type. [...]
>>> -^-
>>>
>>> A "pointer-to-void" is not a "pointer to an object type", because "void"
>>> is not an object type -- it is an incomplete type that cannot be completed:
>>>
>>> -v-
>>> 6.2.5 Types
>>>
>>> 1 [...] Types are partitioned into object types (types that fully
>>> describe objects), function types (types that describe functions), and
>>> incomplete types (types that describe objects but lack information
>>> needed to determine their sizes).
>>>
>>> [...]
>>>
>>> 19 The void type comprises an empty set of values; it is an incomplete
>>> type that cannot be completed.
>>> -^-
>>>
>>> For a different illustration, (vdev->rom + addr) is equivalent to
>>> &(vdev->rom[addr]) -- and we clearly can't have an "array of void".
>>>
>>> This anti-pattern (of doing pointer arithmetic on (void*)) likely comes
>>> from a guarantee that the standard does make, in the same "6.2.5 Types"
>>> section:
>>>
>>> -v-
>>> 27 A pointer to void shall have the same representation and alignment
>>> requirements as a pointer to a character type. 39) [...]
>>>
>>> Footnote 39: The same representation and alignment requirements are
>>> meant to imply interchangeability as arguments to
>>> functions, return values from functions, and members of
>>> unions.
>>> -^-
>>>
>>> It does not extend to the "+" operator.
>>
>> GNU C specifically allows arithmetic on pointers and defines the size
>> of a void as 1. I'll comply, but this makes me want to stab myself in
>> the face :-\ Thanks,
>
> We rely on GNU C extensions all over theplace. Making the code uglier
> to avoid relying on this one here makes no sense to me.
>
I agree, in fact. If GNU-isms are liberally used & tolerated in the QEMU
source, then there's no reason to diverge from that here. I steer clear
of GNU-isms as much as I can, regardless of codebase, but I *did* forget
that QEMU permits GNU-isms -- so there's no need for my pedantry here.
Reviewed-by: Laszlo Ersek <address@hidden>
Thanks!
Laszlo