Bug 71120 - [6/7 Regression] Aliasing "struct sockaddr_storage" produces invalid code due to SRA
Summary: [6/7 Regression] Aliasing "struct sockaddr_storage" produces invalid code due...
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 6.1.1
: P3 normal
Target Milestone: 6.2
Assignee: Martin Jambor
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-15 07:55 UTC by Vincent Bernat
Modified: 2016-05-18 07:36 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work: 5.3.0
Known to fail: 6.1.0
Last reconfirmed: 2016-05-17 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Vincent Bernat 2016-05-15 07:55:03 UTC
Hello,

Starting with gcc-6, the following code (PoC found by Cyril Bonté) will produce an unexpected result:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <arpa/inet.h>

struct dbg_listener {
        struct sockaddr_storage addr;
};

int main(int argc, char **argv)
{
        struct dbg_listener *l;
        struct sockaddr_storage ss, *ss2, ss3;

        memset(&ss3, 0, sizeof(ss3));
        ss3.ss_family = AF_INET;
        ((struct sockaddr_in *)&ss3)->sin_addr.s_addr = inet_addr("127.0.0.1");

        ss2 = &ss3;

        ss = *ss2;

        l = calloc(1, sizeof(*l));
        l->addr = ss;

        printf("%x\n", ntohl(((struct sockaddr_in *)(&l->addr))->sin_addr.s_addr));
        exit(0);
}

This is a common pattern when handling socket addresses in a generic way. Even when the code is compiled with -fno-strict-aliasing, the output is 0 while it should be 7f000001.

$ gcc-6 -O2 -Wall -fno-strict-aliasing debug.c -o debug && ./debug
0

Using -O0 fixes the problem. Fixing the aliasing violation with an union fixes the problem too. Using -fno-tree-sra fixes the problem too. Compiling with gcc-5.3.1 fixes the problem too.

Here is a good starting point for the context of this bug (in HAProxy): http://article.gmane.org/gmane.comp.web.haproxy/27924
Comment 1 Richard Biener 2016-05-17 08:47:54 UTC
Confirmed.  ESRA replaces the aggregate copy

  ss = ss3;

which enables FRE1 to do "interesting" things:

  memset (&ss3, 0, 128);
  ss3.ss_family = 2;
  _6 = inet_addr ("127.0.0.1");
  MEM[(struct sockaddr_in *)&ss3].sin_addr.s_addr = _6;
  ss = ss3;
  l_11 = calloc (1, 128);
  MEM[(struct dbg_listener *)l_11] = 2;
  MEM[(struct dbg_listener *)l_11 + 8B] = 0;
  MEM[(struct dbg_listener *)l_11 + 16B] = 0;
  MEM[(struct dbg_listener *)l_11 + 17B] = 0;
...

so it seems to reach the memset, ignoring the sin_addr.s_addr store.

Mine.
Comment 2 Richard Biener 2016-05-17 15:57:53 UTC
Actually the issue is that SRA decomposes the aggregate assignment in a way
leaving the "interesting" piece out.  sin_addr.s_addr is at offset 4 bytes
and 4 bytes in size while the scalarization

  ss$ss_family_13 = MEM[(struct sockaddr_storage *)&ss3];
  ss$__ss_align_4 = MEM[(struct sockaddr_storage *)&ss3 + 8B];

fails to cover that area.  It scalarizes

struct sockaddr_storage
  {
    sa_family_t ss_family;
    unsigned long int __ss_align;
    char __ss_padding[(128 - (2 * sizeof (unsigned long int)))];
  };

but the actual data is of type

struct sockaddr_in
  {
    sa_family_t sin_family;
    in_port_t sin_port;
    struct in_addr sin_addr;


    unsigned char sin_zero[sizeof (struct sockaddr) -
      (sizeof (unsigned short int)) -
      sizeof (in_port_t) -
      sizeof (struct in_addr)];
  };

so somehow it applies "strict aliasing" rules when fully scalarizing the
block copy.  That needs to be fixed (with -fno-strict-aliasing an
aggregate copy needs to copy all padding).  In this case the scalarization
is also very inefficiently using chars around the calloc call which
will force us to spill all the regs anyway.

All this worked in GCC 5 because somehow we didn't consider to scalarize
ss in the end:

Candidate (4191): ss
Rejected (4190): not aggregate: l
! Disqualifying ss - No scalar replacements to be created.

but I think we're just lucky here.  Relevant accesses should be built from
the aggregate assignment

  l_11->addr = ss;

which is again of sockaddr_storage type.  sockaddr_storage should really have
the may_alias attribute (but in this case this won't help I think).

To recap, with -fno-strict-aliasing the code is valid and should work.
With -fstrict-aliasing the code is bogus and should use memcpy and not
aggregate assignment for both ss = *ss2 and l->addr = ss.
Comment 3 Martin Jambor 2016-05-17 16:00:45 UTC
I will have a look
Comment 4 Richard Biener 2016-05-18 07:32:01 UTC
Actually the bug is in the testcase as it aggregate-copies

struct sockaddr_storage
  {
    sa_family_t ss_family;
    unsigned long int __ss_align;
    char __ss_padding[(128 - (2 * sizeof (unsigned long int)))];
  };

where by C rules an aggregate copy copies each field and thus leaves padding
undefined.

Thus this is a libc bug of having padding between ss_family and __ss_align.

And thus this has nothing to do with strict aliasing.  It's rather a glibc
bug (or specification bug wherever sockaddr_storage is specified).
Comment 5 Richard Biener 2016-05-18 07:36:46 UTC
https://sourceware.org/bugzilla/show_bug.cgi?id=20111