Re: -fno-strict-aliasing and strict-aliasing rules

[David Fifield <david () bamsoftware com>]

On Tue, Jul 21, 2009 at 07:06:13PM -0600, David Fifield wrote:
> On Mon, Jul 20, 2009 at 04:02:56AM +0400, Solar Designer wrote:
> > Attached is a patch for:
> >
> > ncat_ssl.c:134: warning: passing arg 2 of `ASN1_item_d2i' from incompatible pointer type
> > ncat_ssl.c:138: warning: passing arg 2 of pointer to function from incompatible pointer type
> >
> >  #if (OPENSSL_VERSION_NUMBER > 0x00907000L)
> >      if (method->it != NULL) {
> >          ext_str = ASN1_item_d2i(NULL,
> > -            (const unsigned char **) &ext->value->data,
> > +            (unsigned char **) &ext->value->data,
> >              ext->value->length, ASN1_ITEM_ptr(method->it));
> >      } else {
> >          ext_str = method->d2i(NULL,
> > -            (const unsigned char **) &ext->value->data,
> > +            (unsigned char **) &ext->value->data,
> >              ext->value->length);
> >      }
> >  #else
> >
> > BTW, those typecasts are also potentially problematic as it relates to C
> > strict aliasing rules.  There are no warnings from gcc on that, but gcc
> > prints those "strict aliasing warnings" in _some_ cases only, depending
> > on the optimizer.  ...Oh, I've just realized that you build with
> > -fno-strict-aliasing, which should be avoiding the warnings too.  Yet
> > this does not fix the underlying problem, which may well bite you when
> > you build with a modern compiler other than gcc.
>
> This is a valid concern. Nmap is built with -fno-strict-aliasing but
> Ncat is not. I only became aware of the meaning of this option a little
> while ago, after I upgraded my compiler and added -Wall to the default
> Ncat flags. With GCC 4.4.0 I started to get
>
> ncat_main.c: In function #main#:
> ncat_main.c:580: warning: dereferencing pointer #srcaddr.60# does break strict-aliasing rules
> ncat_main.c:580: note: initialized from here
> ncat_main.c:586: warning: dereferencing pointer #srcaddr.62# does break strict-aliasing rules
> ncat_main.c:586: note: initialized from here
> ncat_main.c:629: warning: dereferencing pointer #sin# does break strict-aliasing rules
> ncat_main.c:214: note: initialized from here
> ncat_main.c:746: warning: dereferencing pointer #sin# does break strict-aliasing rules
>
> I guess that GCC 4.4.0 is being more aggressive about strict aliasing
> optimizations, because I didn't get these errors with -Wall before. The
> code it references is code like this:
>
>       struct sockaddr_in *sin = (struct sockaddr_in *) &targetss;
>
> Which is very common practice and blessed by some standards, like RFC 3493:
>
>       "[sockaddr_storage is] aligned at an appropriate boundary so
>       that pointers to it can be cast as pointers to protocol specific
>       address structures and used to access the fields of those
>       structures without alignment problems."
>
> But it is technically an aliasing violation.

I changed this code to use a new union:

union sockaddr_u {
    struct sockaddr_storage storage;
    struct sockaddr_in in;
    struct sockaddr_in6 in6;
    struct sockaddr sockaddr;
};

You use the members of the union instead of casting pointers. For
example, if you have a sockaddr_storage ss, and you want the s_addr from
the sockaddr_in you know is in it, you would do
    ((struct sockaddr_in *) &ss)->sin_addr.s_addr
That's an aliasing violation because you're dereferencing an
incompatible pointer type. With the union, you would write
    su.in.sin_addr.s_addr
With this change, Ncat compiles without warning using -Wstrict-aliasing=2,
notably without the warnings that were printed with only -Wall before.

This technique still seems iffy to me because my copy of The C
Programming Language says "It is the programmer's responsibility to keep
track of which type is currently stored in a union; the results are
implementation-dependent if something is stored as one type and
extracted as another." At least "implementation-defined" is better than
"undefined," because it means the compiler has to document what it does.
This article:
    http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html
says "However, it is an extremely common idiom and is well-supported by
all major compilers. As a practical matter, reading and writing to any
member of a union, in any order, is acceptable practice." It is also the
technique recommended at
    https://bugzilla.redhat.com/show_bug.cgi?id=448743

This strict aliasing business is still a bit mysterious to me and I'm
not sure Ncat has it exactly right. Nsock, for example, does lots of
struct sockaddr pointer casting, but it gives no error with -Wall and
-O2, when -fstrict-aliasing is in effect. I think this is because when
it casts a pointer, it does it when the pointed-to object is not in
scope. The Cell Performance article linked above mentions this, under
"GCC RULE BREAKING": "GCC allows type-punned values to be deferenced at
independent locations in memory (i.e. different objects) when the source
of the lvalue is not directly known." So it may be that these are still
technically aliasing violations, but that GCC is letting us get away
with it. If so, the code in ncat_hostmatch.c need to be adapted to use
sockaddr_u too.

David Fifield

_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://SecLists.Org