[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 5/7] Disable cast-align warnings in various places



On 04/03/2013 09:06 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> There are a number of places which generate cast alignment
> warnings, which are difficult or impossible to address. Use
> pragmas to disable the warnings in these few places

Sheesh - this is some hairy code.

> 
> conf/nwfilter_conf.c: In function 'virNWFilterRuleDetailsParse':
> conf/nwfilter_conf.c:1806:16: warning: cast increases required alignment of target type [-Wcast-align]
>          item = (nwItemDesc *)((char *)nwf + att[idx].dataIdx);

nwf is type virNWFilterRuleDefPtr, and att[idx].dataIdx is populated to
be the offset of a field within virNWFilterRuleDef.  I looked for all
instances of dataIdx initializations, and it loos like all of those
fields are indeed typed nwItemDesc, so the cast is aligned, and I can
agree to the use of a pragma instead of rewriting this.

> conf/nwfilter_conf.c: In function 'virNWFilterRuleDefDetailsFormat':
> conf/nwfilter_conf.c:3238:16: warning: cast increases required alignment of target type [-Wcast-align]
>          item = (nwItemDesc *)((char *)def + att[i].dataIdx);

Same analysis.

> 
> storage/storage_backend_mpath.c: In function 'virStorageBackendCreateVols':
> storage/storage_backend_mpath.c:247:17: warning: cast increases required alignment of target type [-Wcast-align]
>          names = (struct dm_names *)(((char *)names) + next);

names is strcut dm_names*, and libdevmapper (stupidly) set up next to be
a byte offset instead of a struct offset.  Serves them right if they
botched alignment, but I agree that we are stuck here using the pragma.

> 
> nwfilter/nwfilter_dhcpsnoop.c: In function 'virNWFilterSnoopDHCPDecode':
> nwfilter/nwfilter_dhcpsnoop.c:994:15: warning: cast increases required alignment of target type [-Wcast-align]
>          pip = (struct iphdr *) pep->eh_data;

pep is virNWFilterSnoopEthHdrPtr, which is ATTRIBUTE_PACKED.  eh_data
within that struct is therefore aligned only to uint16_t boundaries.
'struct iphdr' in <netinet/ip.h> requires uint32_t alignment.

This is a real case of misalignment.  I'd rather see us work around this
with memcpy() than risk SIGBUS if we read a 32-bit entity that crosses a
word boundary due to 16-bit alignment.

> nwfilter/nwfilter_dhcpsnoop.c:1004:11: warning: cast increases required alignment of target type [-Wcast-align]
>      pup = (struct udphdr *) ((char *) pip + (pip->ihl << 2));

pip is struct iphdr *, which I already said was 32-bit aligned; and the
offset (pip->ihl << 2) is guaranteed to be a multiple of 32-bit
alignment.  Furthermore, struct udphdr appears to be only 16-bit aligned
(the warning about alignment increase is due to the intermediate use of
8-bit char*).  This one is safe for use of the pragma.

> 
> nwfilter/nwfilter_learnipaddr.c: In function 'procDHCPOpts':
> nwfilter/nwfilter_learnipaddr.c:327:33: warning: cast increases required alignment of target type [-Wcast-align]
>                  uint32_t *tmp = (uint32_t *)&dhcpopt->value;

dhcpopt is declared as struct dhcp_option, which is only 8-bit aligned
(it has a pointless ATTRIBUTE_PACKED, given that none of its fields are
larger than char).  But dhcpopt is created as uint16_t away from the
argument dhcp; and the only caller is at line 562, where dhcp was
computed based on a udphdr.  Sounds like it is an alignment problem
after all, and I'd rather see memcpy() used to avoid the possibility of
a SIGBUS.

> nwfilter/nwfilter_learnipaddr.c: In function 'learnIPAddressThread':
> nwfilter/nwfilter_learnipaddr.c:501:43: warning: cast increases required alignment of target type [-Wcast-align]
>                      struct iphdr *iphdr = (struct iphdr*)(packet +

packet is merely a byte array with no alignment (in reality, it's
probably at least uint16_t aligned, and maybe we get lucky with uint32_t
alignment; but I don't know that pcap_next() guarantees that).  Here,...

> nwfilter/nwfilter_learnipaddr.c:538:43: warning: cast increases required alignment of target type [-Wcast-align]
>                      struct iphdr *iphdr = (struct iphdr*)(packet +
> nwfilter/nwfilter_learnipaddr.c:544:48: warning: cast increases required alignment of target type [-Wcast-align]
>                          struct udphdr *udphdr= (struct udphdr *)

...and in these two as well, we are progressively looking further into
that raw array of packet, and trying to parse out possibly unaligned
data.  I have no idea how we could possibly guarantee alignment, so the
pragma may be the best we can do without writing the code to access
unaligned offsets a byte at a time instead of as a struct field
dereference.  Let's face it - trying to parse raw packets off a NIC is
painful when you can't use unaligned access.

Ultimately, I think this file would be better if we used accessors from
virendian.h (well, we'd have to add virReadBufInt16BE/virReadBufInt16LE)
to read integers from byte[]+offset, (the virendian.h macros are safe
regardless of alignment) instead of trying to cast through a packed type
and risking problems.  But doing a rewrite like that is a much bigger
task; so what you have at least shuts the compiler up, even if it
doesn't fix the issues at hand.

> 
> Signed-off-by: Daniel P. Berrange <berrange redhat com>
> ---
>  m4/virt-compile-warnings.m4         |  1 +
>  src/conf/nwfilter_conf.c            |  4 ++++
>  src/internal.h                      | 13 +++++++++++++
>  src/nwfilter/nwfilter_dhcpsnoop.c   |  4 ++++
>  src/nwfilter/nwfilter_learnipaddr.c | 10 ++++++++++
>  src/storage/storage_backend_mpath.c |  2 ++
>  6 files changed, 34 insertions(+)
> 
> diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
> index a08fcac..e054913 100644
> --- a/m4/virt-compile-warnings.m4
> +++ b/m4/virt-compile-warnings.m4
> @@ -93,6 +93,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
>      if test $lv_cv_gcc_pragma_push_works = no; then
>        dontwarn="$dontwarn -Wmissing-prototypes"
>        dontwarn="$dontwarn -Wmissing-declarations"
> +      dontwarn="$dontwarn -Wcast-align"
>      fi

Good.

>  
>      dnl Check whether strchr(s, char variable) causes a bogus compile
> diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
> index e63a04b..b61010d 100644
> --- a/src/conf/nwfilter_conf.c
> +++ b/src/conf/nwfilter_conf.c
> @@ -1803,7 +1803,9 @@ virNWFilterRuleDetailsParse(xmlNodePtr node,
>      while (att[idx].name != NULL) {
>          prop = virXMLPropString(node, att[idx].name);
>  
> +        VIR_WARNINGS_NO_CAST_ALIGN
>          item = (nwItemDesc *)((char *)nwf + att[idx].dataIdx);
> +        VIR_WARNINGS_RESET

Good (see my comments above).

>          flags = &item->flags;
>          flags_set = match_flag;
>  
> @@ -3235,7 +3237,9 @@ virNWFilterRuleDefDetailsFormat(virBufferPtr buf,
>      nwItemDesc *item;
>  
>      while (att[i].name) {
> +        VIR_WARNINGS_NO_CAST_ALIGN
>          item = (nwItemDesc *)((char *)def + att[i].dataIdx);
> +        VIR_WARNINGS_RESET

Good.

>          enum virNWFilterEntryItemFlags flags = item->flags;
>          if ((flags & NWFILTER_ENTRY_ITEM_FLAG_EXISTS)) {
>              if (!typeShown) {
> diff --git a/src/internal.h b/src/internal.h
> index 2cf4731..d819aa3 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -214,6 +214,19 @@
>  #  endif
>  # endif				/* __GNUC__ */
>  
> +
> +# if __GNUC_PREREQ (4, 6)
> +#  define VIR_WARNINGS_NO_CAST_ALIGN \
> +    _Pragma ("GCC diagnostic push") \
> +    _Pragma ("GCC diagnostic ignored \"-Wcast-align\"")
> +
> +#  define VIR_WARNINGS_RESET \
> +    _Pragma ("GCC diagnostic pop")
> +# else
> +#  define VIR_WARNINGS_NO_CAST_ALIGN
> +#  define VIR_WARNINGS_RESET
> +# endif
> +

Good.

>  /*
>   * Use this when passing possibly-NULL strings to printf-a-likes.
>   */
> diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
> index 5124069..5012b14 100644
> --- a/src/nwfilter/nwfilter_dhcpsnoop.c
> +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
> @@ -991,7 +991,9 @@ virNWFilterSnoopDHCPDecode(virNWFilterSnoopReqPtr req,
>      /* go through the protocol headers */
>      switch (ntohs(pep->eh_type)) {
>      case ETHERTYPE_IP:
> +        VIR_WARNINGS_NO_CAST_ALIGN;
>          pip = (struct iphdr *) pep->eh_data;
> +        VIR_WARNINGS_RESET;

Tolerable, although I'd prefer a memcpy or virendian.h solution for
whatever we end up reading out of pip.

>          len -= offsetof(virNWFilterSnoopEthHdr, eh_data);
>          break;
>      default:
> @@ -1001,7 +1003,9 @@ virNWFilterSnoopDHCPDecode(virNWFilterSnoopReqPtr req,
>      if (len < 0)
>          return -2;
>  
> +    VIR_WARNINGS_NO_CAST_ALIGN
>      pup = (struct udphdr *) ((char *) pip + (pip->ihl << 2));
> +    VIR_WARNINGS_RESET

Good.

>      len -= pip->ihl << 2;
>      if (len < 0)
>          return -2;
> diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
> index 7a4f983..b399092 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.c
> +++ b/src/nwfilter/nwfilter_learnipaddr.c
> @@ -324,7 +324,9 @@ procDHCPOpts(struct dhcp *dhcp, int dhcp_opts_len,
>  
>          case DHCP_OPT_BCASTADDRESS: /* Broadcast address */
>              if (dhcp_opts_len >= 6) {
> +                VIR_WARNINGS_NO_CAST_ALIGN
>                  uint32_t *tmp = (uint32_t *)&dhcpopt->value;
> +                VIR_WARNINGS_RESET

Tolerable.

>                  (*bcastaddr) = ntohl(*tmp);
>              }
>          break;
> @@ -498,8 +500,10 @@ learnIPAddressThread(void *arg)
>                  if (etherType == ETHERTYPE_IP &&
>                      (header.len >= ethHdrSize +
>                                     sizeof(struct iphdr))) {
> +                    VIR_WARNINGS_NO_CAST_ALIGN
>                      struct iphdr *iphdr = (struct iphdr*)(packet +
>                                                            ethHdrSize);
> +                    VIR_WARNINGS_RESET

Tolerable.

>                      vmaddr = iphdr->saddr;
>                      /* skip mcast addresses (224.0.0.0 - 239.255.255.255),
>                       * class E (240.0.0.0 - 255.255.255.255, includes eth.
> @@ -514,8 +518,10 @@ learnIPAddressThread(void *arg)
>                  } else if (etherType == ETHERTYPE_ARP &&
>                             (header.len >= ethHdrSize +
>                                            sizeof(struct f_arphdr))) {
> +                    VIR_WARNINGS_NO_CAST_ALIGN
>                      struct f_arphdr *arphdr = (struct f_arphdr*)(packet +
>                                                           ethHdrSize);
> +                    VIR_WARNINGS_RESET

Tolerable.

>                      switch (ntohs(arphdr->arphdr.ar_op)) {
>                      case ARPOP_REPLY:
>                          vmaddr = arphdr->ar_sip;
> @@ -535,14 +541,18 @@ learnIPAddressThread(void *arg)
>                  if (etherType == ETHERTYPE_IP &&
>                      (header.len >= ethHdrSize +
>                                     sizeof(struct iphdr))) {
> +                    VIR_WARNINGS_NO_CAST_ALIGN
>                      struct iphdr *iphdr = (struct iphdr*)(packet +
>                                                            ethHdrSize);
> +                    VIR_WARNINGS_RESET

Tolerable.

>                      if ((iphdr->protocol == IPPROTO_UDP) &&
>                          (header.len >= ethHdrSize +
>                                         iphdr->ihl * 4 +
>                                         sizeof(struct udphdr))) {
> +                        VIR_WARNINGS_NO_CAST_ALIGN
>                          struct udphdr *udphdr= (struct udphdr *)
>                                            ((char *)iphdr + iphdr->ihl * 4);
> +                        VIR_WARNINGS_RESET

Tolerable.

>                          if (ntohs(udphdr->source) == 67 &&
>                              ntohs(udphdr->dest)   == 68 &&
>                              header.len >= ethHdrSize +
> diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c
> index b12b81f..6049063 100644
> --- a/src/storage/storage_backend_mpath.c
> +++ b/src/storage/storage_backend_mpath.c
> @@ -243,8 +243,10 @@ virStorageBackendCreateVols(virStoragePoolObjPtr pool,
>  
>          /* Given the way libdevmapper returns its data, I don't see
>           * any way to avoid this series of casts. */
> +        VIR_WARNINGS_NO_CAST_ALIGN
>          next = names->next;
>          names = (struct dm_names *)(((char *)names) + next);
> +        VIR_WARNINGS_RESET

No choice in the matter; good.

ACK.  Although I wouldn't mind if you attempt a v2 to address some of
the more questionable locations, I also won't insist on it right now.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]