Host: Mac OS 10.12.5
Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)

  slirp/ip6_icmp.c:79:32: warning: taking address of packed member 'ip_src' of class or
        structure 'ip6' may result in an unaligned pointer value
        [-Waddress-of-packed-member]
      if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
                                 ^~~~~~~~~~
  /usr/include/netinet6/in6.h:299:36: note: expanded from macro 'IN6_IS_ADDR_MULTICAST'
  #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)
                                            ^

Reported-by: John Arbuckle <programmingkidx@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 slirp/ip6.h       |  5 +++++
 slirp/ip6_icmp.c  | 10 +++++-----
 slirp/ndp_table.c |  4 ++--
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/slirp/ip6.h b/slirp/ip6.h
index b1bea43b3c..6c5d4eeaa3 100644
--- a/slirp/ip6.h
+++ b/slirp/ip6.h
@@ -93,6 +93,11 @@ static inline bool in6_equal_mach(const struct in6_addr *a,
 #define in6_zero(a)\
     (in6_equal(a, &(struct in6_addr)ZERO_ADDR))
 
+static inline bool in6_multicast(const struct in6_addr *a)
+{
+    return a->s6_addr[0] == 0xff;
+}
+
 /* Compute emulated host MAC address from its ipv6 address */
 static inline void in6_compute_ethaddr(struct in6_addr ip,
                                        uint8_t eth[ETH_ALEN])
diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index ee333d05a2..9796624648 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -76,7 +76,7 @@ void icmp6_send_error(struct mbuf *m, uint8_t type, uint8_t code)
     DEBUG_CALL("icmp6_send_error");
     DEBUG_ARGS((dfd, " type = %d, code = %d\n", type, code));
 
-    if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
+    if (in6_multicast(&ip->ip_src) ||
             in6_zero(&ip->ip_src)) {
         /* TODO icmp error? */
         return;
@@ -291,7 +291,7 @@ static void ndp_send_na(Slirp *slirp, struct ip6 *ip, struct icmp6 *icmp)
 
     /* NDP */
     ricmp->icmp6_nna.R = NDP_IsRouter;
-    ricmp->icmp6_nna.S = !IN6_IS_ADDR_MULTICAST(&rip->ip_dst);
+    ricmp->icmp6_nna.S = !in6_multicast(&rip->ip_dst);
     ricmp->icmp6_nna.O = 1;
     ricmp->icmp6_nna.reserved_hi = 0;
     ricmp->icmp6_nna.reserved_lo = 0;
@@ -348,7 +348,7 @@ static void ndp_input(struct mbuf *m, Slirp *slirp, struct ip6 *ip,
         DEBUG_CALL(" type = Neighbor Solicitation");
         if (ip->ip_hl == 255
                 && icmp->icmp6_code == 0
-                && !IN6_IS_ADDR_MULTICAST(&icmp->icmp6_nns.target)
+                && !in6_multicast(&icmp->icmp6_nns.target)
                 && ntohs(ip->ip_pl) >= ICMP6_NDP_NS_MINLEN
                 && (!in6_zero(&ip->ip_src)
                     || in6_solicitednode_multicast(&ip->ip_dst))) {
@@ -365,8 +365,8 @@ static void ndp_input(struct mbuf *m, Slirp *slirp, struct ip6 *ip,
         if (ip->ip_hl == 255
                 && icmp->icmp6_code == 0
                 && ntohs(ip->ip_pl) >= ICMP6_NDP_NA_MINLEN
-                && !IN6_IS_ADDR_MULTICAST(&icmp->icmp6_nna.target)
-                && (!IN6_IS_ADDR_MULTICAST(&ip->ip_dst)
+                && !in6_multicast(&icmp->icmp6_nna.target)
+                && (!in6_multicast(&ip->ip_dst)
                     || icmp->icmp6_nna.S == 0)) {
             ndp_table_add(slirp, ip->ip_src, eth->h_source);
         }
diff --git a/slirp/ndp_table.c b/slirp/ndp_table.c
index e1676a0a7b..5ff4bf2f3d 100644
--- a/slirp/ndp_table.c
+++ b/slirp/ndp_table.c
@@ -23,7 +23,7 @@ void ndp_table_add(Slirp *slirp, struct in6_addr ip_addr,
                 ethaddr[0], ethaddr[1], ethaddr[2],
                 ethaddr[3], ethaddr[4], ethaddr[5]));
 
-    if (IN6_IS_ADDR_MULTICAST(&ip_addr) || in6_zero(&ip_addr)) {
+    if (in6_multicast(&ip_addr) || in6_zero(&ip_addr)) {
         /* Do not register multicast or unspecified addresses */
         DEBUG_CALL(" abort: do not register multicast or unspecified address");
         return;
@@ -63,7 +63,7 @@ bool ndp_table_search(Slirp *slirp, struct in6_addr ip_addr,
     assert(!in6_zero(&ip_addr));
 
     /* Multicast address: fec0::abcd:efgh/8 -> 33:33:ab:cd:ef:gh */
-    if (IN6_IS_ADDR_MULTICAST(&ip_addr)) {
+    if (in6_multicast(&ip_addr)) {
         out_ethaddr[0] = 0x33; out_ethaddr[1] = 0x33;
         out_ethaddr[2] = ip_addr.s6_addr[12];
         out_ethaddr[3] = ip_addr.s6_addr[13];
-- 
2.15.1

On 08.01.2018 18:28, Philippe Mathieu-Daudé wrote:
> Host: Mac OS 10.12.5
> Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
> 
>   slirp/ip6_icmp.c:79:32: warning: taking address of packed member 'ip_src' of class or
>         structure 'ip6' may result in an unaligned pointer value
>         [-Waddress-of-packed-member]
>       if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
>                                  ^~~~~~~~~~
>   /usr/include/netinet6/in6.h:299:36: note: expanded from macro 'IN6_IS_ADDR_MULTICAST'
>   #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)
>                                             ^
> 
> Reported-by: John Arbuckle <programmingkidx@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  slirp/ip6.h       |  5 +++++
>  slirp/ip6_icmp.c  | 10 +++++-----
>  slirp/ndp_table.c |  4 ++--
>  3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/slirp/ip6.h b/slirp/ip6.h
> index b1bea43b3c..6c5d4eeaa3 100644
> --- a/slirp/ip6.h
> +++ b/slirp/ip6.h
> @@ -93,6 +93,11 @@ static inline bool in6_equal_mach(const struct in6_addr *a,
>  #define in6_zero(a)\
>      (in6_equal(a, &(struct in6_addr)ZERO_ADDR))

I think you should put a comment here to say why we need our own
function and can not use the IN6_IS_ADDR_MULTICAST macro instead -
otherwise people might be confused when looking at this code in a year
or two. (and now I've also understood why you're poisining the macros in
the next patch ... a comment in the code there would certainly not hurt
either).

> +static inline bool in6_multicast(const struct in6_addr *a)
> +{
> +    return a->s6_addr[0] == 0xff;
> +}
> +
>  /* Compute emulated host MAC address from its ipv6 address */
>  static inline void in6_compute_ethaddr(struct in6_addr ip,
>                                         uint8_t eth[ETH_ALEN])

 Thomas

On 01/08/2018 09:28 AM, Philippe Mathieu-Daudé wrote:
> Host: Mac OS 10.12.5
> Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
> 
>   slirp/ip6_icmp.c:79:32: warning: taking address of packed member 'ip_src' of class or
>         structure 'ip6' may result in an unaligned pointer value
>         [-Waddress-of-packed-member]
>       if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
>                                  ^~~~~~~~~~
>   /usr/include/netinet6/in6.h:299:36: note: expanded from macro 'IN6_IS_ADDR_MULTICAST'
>   #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)

The fact that you replace a macro with a function of exactly the same code in
order to avoid a diagnostic sure looks like a compiler bug to me.


r~

On 09.01.2018 17:33, Richard Henderson wrote:
> On 01/08/2018 09:28 AM, Philippe Mathieu-Daudé wrote:
>> Host: Mac OS 10.12.5
>> Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
>>
>>   slirp/ip6_icmp.c:79:32: warning: taking address of packed member 'ip_src' of class or
>>         structure 'ip6' may result in an unaligned pointer value
>>         [-Waddress-of-packed-member]
>>       if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
>>                                  ^~~~~~~~~~
>>   /usr/include/netinet6/in6.h:299:36: note: expanded from macro 'IN6_IS_ADDR_MULTICAST'
>>   #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)
> 
> The fact that you replace a macro with a function of exactly the same code in
> order to avoid a diagnostic sure looks like a compiler bug to me.

I think this might also be a bug in the macro definitions. On Linux, it
is defined like this:

#define IN6_IS_ADDR_MULTICAST(a) (((const uint8_t *) (a))[0] == 0xff)

 Thomas

Richard Henderson, on mar. 09 janv. 2018 08:33:22 -0800, wrote:
> On 01/08/2018 09:28 AM, Philippe Mathieu-Daudé wrote:
> > Host: Mac OS 10.12.5
> > Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
> > 
> >   slirp/ip6_icmp.c:79:32: warning: taking address of packed member 'ip_src' of class or
> >         structure 'ip6' may result in an unaligned pointer value
> >         [-Waddress-of-packed-member]
> >       if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
> >                                  ^~~~~~~~~~
> >   /usr/include/netinet6/in6.h:299:36: note: expanded from macro 'IN6_IS_ADDR_MULTICAST'
> >   #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)
> 
> The fact that you replace a macro with a function of exactly the same code in
> order to avoid a diagnostic sure looks like a compiler bug to me.

The compiler could be smarter to realize that it's actually a byte access indeed.

There are two things for me:

- The OS-provided implementation of IN6_IS_ADDR_MULTICAST could be doing
  32bit accesses and whatnot, so we are not supposed to use it on packed
  fields.

- With the proposal patch, the compiler could still emit the warning,
  since we are basically still passing the address of the packed field
  to a function which the compiler might not see either.  We are however
  sure that the function makes an aligned access.

So I'd say we should still rather do it to be on the safe side. With
Thomas Huth's comments applied.

BTW,

Thomas Huth <thuth@redhat.com> wrote:
> I think this might also be a bug in the macro definitions.

> > > #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)

> On Linux, it is defined like this:
> 
> #define IN6_IS_ADDR_MULTICAST(a) (((const uint8_t *) (a))[0] == 0xff)

Well, the standard says that IN6_IS_ADDR_MULTICAST takes a struct
in6_addr*, so it can use the s6_addr field.

Samuel