Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read

[Laszlo Ersek <lersek@redhat.com>]

On 03/11/20 08:08, Markus Armbruster wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
>> On Wed, 11 Mar 2020 00:14:31 +0100
>> Laszlo Ersek <lersek@redhat.com> 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 <lersek@redhat.com>

Thanks!
Laszlo