qemu-devel
[Top][All Lists]
Advanced

[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: Alex Williamson
Subject: Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read
Date: Wed, 11 Mar 2020 09:56:43 -0600

On Wed, 11 Mar 2020 11:28:26 +0100
Laszlo Ersek <address@hidden> wrote:

> 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>

Oh, thank goodness ;)  Thanks,

Alex




reply via email to

[Prev in Thread] Current Thread [Next in Thread]