GCC warning about array out of bounds, false positive?

Question

I have this simple function

#define IP_AS_V6(storage, f) ((struct sockaddr_in6 *)&(storage))->sin6_##f
static inline int ip_check_equal_v6
  (const struct sockaddr_storage *a, const struct sockaddr_storage *b)
    { return ((uint64_t *)IP_AS_V6(a, addr).s6_addr)[0] == ((uint64_t *)IP_AS_V6(b, addr).s6_addr)[0] &&
             ((uint64_t *)IP_AS_V6(a, addr).s6_addr)[1] == ((uint64_t *)IP_AS_V6(b, addr).s6_addr)[1]; }

and after switching to GCC-12 it no longer compiles because of

error: array subscript 2 is outside array bounds of 'const struct sockaddr_storage[0]' [-Werror=array-bounds]
    5 |              ((uint64_t *)IP_AS_V6(a, addr).s6_addr)[1] == ((uint64_t *)IP_AS_V6(b, addr).s6_addr)[1]; }
      |                                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~

An IPv6 address is 128 bits, so when looking at it as a 64 bit int array the index 1 should be perfectly fine. Is this code illegal/undefined for some other reason or is this indeed a bug in gcc-12?

Wait. `b` is a pointer, then your macro takes a pointer *to* that pointer, casts the pointer-to-pointer to a pointer-to-struct, and tries to access a struct member? — user2357112, Sep 15 '22 at 00:54
Lost in the casting... Have you considered `memcmp()`?... Useful library function that may be a tad slower, but more obvious (and perhaps portable.) — Fe2O3, Sep 15 '22 at 00:58
*An IPv6 address is 128 bits, so when looking at it as a 64 bit int array the index 1 should be perfectly fine.* That's not true. In general, "looking at" any type of variable as any other type (other than `char`) is [a strict-aliasing violation and undefined behavior](https://stackoverflow.com/questions/98650/what-is-the-strict-aliasing-rule). — Andrew Henle, Sep 15 '22 at 01:18

John Bollinger · Accepted Answer · 2022-09-15T13:25:54.957

Let's look at what your macro is actually doing. Given ...

const struct sockaddr_storage *a;

... the expression (uint64_t *)IP_AS_V6(a, addr).s6_addr)[0] expands to...

(uint64_t *)((struct sockaddr_in6 *)&(a))->sin6_addr.s6_addr)[0]

In the first place, you've got your levels of indirection messed up, at least if a in fact points to a struct sockaddr_in6. &(a) is a pointer to the struct pointer, which is of no use for your purpose at all. You presumably meant just (a), not its address. This is probably the origin of the compiler's complaint.

In the second place, (struct sockaddr_in6 *)(a))->sin6_addr.s6_addr has type unsigned char[16]. That is automatically converted to an unsigned char * which you might safely be able to convert to type uint64_t *, but then again you might not. If the resulting pointer is not correctly aligned for a uint64_t * then undefined behavior results, and it's not clear to me that there's anything to ensure that the alignment works out.

In the third place, even if the alignment worked out, accessing parts of an unsigned char[] via an lvalue of type uint64_t is a violation of the strict aliasing rule, producing undefined behavior.

The function also casts away constness, which is poor form and should elicit a warning, even though it doesn't try to modify either object.

I think I would write the function this way:

static inline int ip_check_equal_v6(const struct sockaddr_storage *a,
        const struct sockaddr_storage *b) {
    const struct sockaddr_in6 *a6 = (const struct sockaddr_in6 *) a;
    const struct sockaddr_in6 *b6 = (const struct sockaddr_in6 *) b;

    return !memcmp(a6->sin6_addr.s6_addr, a6->sin6_addr.s6_addr,
                   sizeof(a6->sin6_addr.s6_addr));
}

That returns 1 if the address bytes in the first address are all the same as those in the second (with both interpreted as IPV6 addresses), and 0 if any of them differ, which appears to be what your version is intended to do. And do note how incredibly much easier than the original it is to read, which makes it much easier to maintain, and even much easier to evaluate by eye. Your compiler might even be able to optimize it better than the original, though I make no promises about that.

@chux-ReinstateMonica, because I took my cue from the OP's version, which compared the space in two 8-byte pieces, without any explicit reference to its declared size. But you're right, it is better -- certainly clearer -- to use the `sizeof` one of the objects being compared. Edited. — John Bollinger, Sep 15 '22 at 13:28

GCC warning about array out of bounds, false positive?

1 Answers1