[systemd-devel] [systemd-commits] 2 commits - src/libsystemd-network src/udev

[Shawn Landden]

On Fri, Apr 10, 2015 at 07:54:33PM +0200, Ronny Chevalier wrote:
> On Fri, Apr 10, 2015 at 7:05 PM, Lennart Poettering
> <lennart at poettering.net> wrote:
> > On Sat, 14.03.15 06:54, Ronny Chevalier (rchevalier at kemper.freedesktop.org) wrote:
> >
> >> commit 6ec8e7c763b7dfa82e25e31f6938122748d1608f
> >> Author: Shawn Landden <shawn at churchofgit.com>
> >> Date:   Tue Mar 10 20:45:15 2015 -0700
> >>
> >>     sd-dhcp-client: fix strict aliasing issue
> >>
> >> diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c
> >> index 4224e01..a477ccc 100644
> >> --- a/src/libsystemd-network/sd-dhcp-client.c
> >> +++ b/src/libsystemd-network/sd-dhcp-client.c
> >> @@ -1469,7 +1469,7 @@ static int client_receive_message_udp(sd_event_source *s, int fd,
> >>          _cleanup_free_ DHCPMessage *message = NULL;
> >>          int buflen = 0, len, r;
> >>          const struct ether_addr zero_mac = { { 0, 0, 0, 0, 0, 0 } };
> >> -        const struct ether_addr *expected_chaddr = NULL;
> >> +        bool expect_chaddr;
> >>          uint8_t expected_hlen = 0;
> >>
> >>          assert(s);
> >> @@ -1514,11 +1514,11 @@ static int client_receive_message_udp(sd_event_source *s, int fd,
> >>
> >>          if (client->arp_type == ARPHRD_ETHER) {
> >>                  expected_hlen = ETH_ALEN;
> >> -                expected_chaddr = (const struct ether_addr *) &client->mac_addr;
> >> +                expect_chaddr = true;
> >>          } else {
> >>                 /* Non-ethernet links expect zero chaddr */
> >>                 expected_hlen = 0;
> >> -               expected_chaddr = &zero_mac;
> >> +               expect_chaddr = false;
> >>          }
> >>
> >>          if (message->hlen != expected_hlen) {
> >> @@ -1526,7 +1526,10 @@ static int client_receive_message_udp(sd_event_source *s, int fd,
> >>                  return 0;
> >>          }
> >>
> >> -        if (memcmp(&message->chaddr[0], expected_chaddr, ETH_ALEN)) {
> >> +        if (memcmp(&message->chaddr[0], expect_chaddr ?
> >> +                                          (void *)&client->mac_addr :
> >> +                                          (void *)&zero_mac,
> >> +                                        ETH_ALEN)) {
> >>                  log_dhcp_client(client, "received chaddr does not match "
> >>                                  "expected: ignoring");
> >>                  return 0;
> >
> > Ahum, what is this about? Please elaborate what kind of struct aliasing
> > this fixes?
> 
> I think Shawn was trying to avoid to cast mac_addr which is (uint8_t
> *) to a (const struct ether_addr *) which breaks the strict aliasing
> rule. Or I misunderstood the patch? Shawn?
> >> -                expected_chaddr = (const struct ether_addr *) &client->mac_addr;
> 
> 
> >
> > The compiler knows very well what we are taking the pointer of... and
> > memcmp() doesn't really care anyway it takes (void*) pointers anyway...
> 
> The issue is not with memcmp but with the cast from a (uint8_t *) to a
> (const struct ether_addr *) which breaks the strict aliasing rule.
Given that we don't do any operations on it (besides memcmp which doesn't matter)
it doesn't actually violate, but it does generate an annoying warning.
> 
> If I'm not mistaken Shawn is trying to remove all the strict aliasing
> violations reported by gcc, in order to add the warning later on.
> 
> Ronny