[libvirt] [PATCH 1/2] nwfilter: Fix pointer.

[Richard W.M. Jones]

On Mon, Jan 21, 2019 at 03:23:59PM +0000, Daniel P. Berrangé wrote:
> On Mon, Jan 21, 2019 at 03:13:20PM +0000, Richard W.M. Jones wrote:
> > GCC 9 complains:
> > 
> > nwfilter/nwfilter_dhcpsnoop.c: In function 'virNWFilterDHCPSnoopThread':
> > nwfilter/nwfilter_dhcpsnoop.c:1456:31: error: converting a packed 'virNWFilterSnoopEthHdrPtr' {aka 'struct _virNWFilterSnoopEthHdr *'} pointer (alignment 1) to 'const u_char *' {aka 'const unsigned char *'} (alignment 8) may result in an unaligned pointer value [-Werror=address-of-packed-member]
> >  1456 |                               (const u_char **)&packet);
> >       |                               ^
> 
> I tend to think this warning is bogus.   We are not de-referencing
> any packed fields within the sctruct when we call to pcap_next_ex()
> with the cast.  pcap_next_ex() is just going to fill the entire
> memory region with a read off the wire, so it would not be triggering
> unaligned access either.  IOW, I don't think the compiler should be
> warning there
> 
> IIUC gcc X.0.0  versions are not in fact relases, but rather
> pre-release snapshots.
> 
> If so, I think this might be a bug that needs reporting against
> the GCC pre-release.
> 
> 
> 
> > nwfilter/nwfilter_dhcpsnoop.c:183:8: note: defined here
> >   183 | struct _virNWFilterSnoopEthHdr {
> >       |        ^~~~~~~~~~~~~~~~~~~~~~~
> > 
> > However it seems like there's more going on here than just an enhanced
> > GCC warning.  The function pcap_next_ex is documented as:
> > 
> >        the pointer pointed  to  by  the
> >        pkt_data  argument  is  set  to  point  to the data in the packet
> > 
> > We are passing a struct here rather than a pointer.  I changed the
> > code to pass a pointer instead.
> 
> > diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
> > index 58f0057c3f..45873a542c 100644
> > --- a/src/nwfilter/nwfilter_dhcpsnoop.c
> > +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
> > @@ -1335,7 +1335,7 @@ virNWFilterDHCPSnoopThread(void *req0)
> >  {
> >      virNWFilterSnoopReqPtr req = req0;
> >      struct pcap_pkthdr *hdr;
> > -    virNWFilterSnoopEthHdrPtr packet;
> > +    const virNWFilterSnoopEthHdrPtr *packetPtr;
> 
> virNWFilterSnoopEthHdrPtr is already a pointer to a virNWFilterSnoopEthHdr.
> 
> So this change turns it into a pointer to a pointer....

Duh you're right there.

Yup as you say this patch is bogus and more likely indicates
some bug in GCC.

Rich.

> >      int ifindex = 0;
> >      int errcount = 0;
> >      int tmp = -1, rv, n, pollTo;
> > @@ -1453,7 +1453,7 @@ virNWFilterDHCPSnoopThread(void *req0)
> >              n--;
> >  
> >              rv = pcap_next_ex(pcapConf[i].handle, &hdr,
> > -                              (const u_char **)&packet);
> > +                              (const u_char **)&packetPtr);
> 
> And then into a pointer to a pointer to a pointer.
> 
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v