[Bug network/16421] New: IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[Bug network/16421] New: IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[kazssym at vx68k dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

            Bug ID: 16421
           Summary: IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32
           Product: glibc
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: network
          Assignee: unassigned at sourceware dot org
          Reporter: kazssym at vx68k dot org

I have got a report that my program causes compile error with undefined
s6_addr32 on the line I use IN6_IS_ADDR_UNSPECIFIED.  I see s6_addr32 is
defined in <netinet/in.h> only if either __USE_MISC or __USE_GNU is defined but
IN6_IS_ADDR_UNSPECIFIED uses s6_addr32 unconditionally on GCC.  I believe it is
a bug in the definition of IN6_IS_ADDR_UNSPECIFIED and similar macros.

Although I can work around the error by defining _GNU_SOURCE in the source
file, I wish it would be fixed as soon as possible.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[bugdal at aerifal dot cx]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

Rich Felker <bugdal at aerifal dot cx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugdal at aerifal dot cx

--- Comment #1 from Rich Felker <bugdal at aerifal dot cx> ---
s6_* is in the reserved namespace for this header (see
http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_02)
so the simplest fix would be to expose s6_addr32 unconditionally.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[kazssym at vx68k dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #2 from kazssym at vx68k dot org ---
(In reply to Rich Felker from comment #1)
> s6_* is in the reserved namespace for this header (see
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.
> html#tag_15_02_02) so the simplest fix would be to expose s6_addr32
> unconditionally.

I don't know whether it is good or not to define non-standard names even if
they aren't requested by application programs.  I am afread that somebody might
try to use them without knowing they are non-standard.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[bugdal at aerifal dot cx]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #3 from Rich Felker <bugdal at aerifal dot cx> ---
Well in that case a reasonable alternative would be to expose __s6_addr32
instead of s6_addr32 in standards-conforming mode.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

Carlos O'Donell <carlos at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |carlos at redhat dot com

--- Comment #5 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Rich Felker from comment #3)
> Well in that case a reasonable alternative would be to expose __s6_addr32
> instead of s6_addr32 in standards-conforming mode.

Andreas Schwab comments[1] that the macros should themselves used the internal
__s6_* names instead of the public ones, that way the implementation is always
internally consistent.

I see no reason why we shouldn't do that. It avoids the problem of always
defining s6_addr32 being problematic. It doesn't require any code to check if
you're in POSIX or non-POSIX conformance modes.

[1] https://sourceware.org/ml/libc-alpha/2013-04/msg00564.html

Comments?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #6 from Carlos O'Donell <carlos at redhat dot com> ---
Created attachment 7387
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7387&action=edit
netinet-in-du.diff

Always use the internal union name within implementation macros to avoid the
requirement on non-POSIX names for the union types.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #7 from Carlos O'Donell <carlos at redhat dot com> ---
Anyone interested in testing the attached patch?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[alvarezp at alvarezp dot ods.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #8 from Octavio Alvarez <alvarezp at alvarezp dot ods.org> ---
That patch will fail. I'll prepare a new one that should work.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[alvarezp at alvarezp dot ods.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

Octavio Alvarez <alvarezp at alvarezp dot ods.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7387|0                           |1
        is obsolete|                            |

--- Comment #9 from Octavio Alvarez <alvarezp at alvarezp dot ods.org> ---
Created attachment 7388
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7388&action=edit
Expose __in6_u.__u6_addr32 and let IN6_* macros use that unconditionally.

This patch:

* Unconditionally exposes __in6_u.__u6_addr32[] (and 16). This
  shouldn't cause any conflicts.

* Removes both previous implementations (POSIX and GNU) and
  redefines the macros to use __in6_u.__u6_addr32[] instead.

* Unconditionally removes the __extension__ qualifier on the
  macros.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[alvarezp at alvarezp dot ods.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #10 from Octavio Alvarez <alvarezp at alvarezp dot ods.org> ---
I just tested if patching my /usr/include/netinet/in.h fixes compilation on our
project. It does. With the patch, the project no longer needs the local
workaround to complete compilation.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #11 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Octavio Alvarez from comment #10)
> I just tested if patching my /usr/include/netinet/in.h fixes compilation on
> our project. It does. With the patch, the project no longer needs the local
> workaround to complete compilation.

Your patch is almost there.

That patch is unacceptable because it exposes code which relies on non-ISO C
extensions. Please make sure you know why you are removing the code you are
removing.

Add back the `#ifdef __GNUC__` and __extension__ usage, and keep the changes to
reference internal types and that would be an acceptable patch to post to
libc-alpha.

Please note that we are frozen for the 2.19 release, so this will go in once
2.20 opens in a week or less.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[alvarezp at alvarezp dot ods.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #12 from Octavio Alvarez <alvarezp at alvarezp dot ods.org> ---
Thank you. Today I have learned about statement expressions and how they are
GCC extensions.

If we add an #ifdef __GNUC__ we would also have to add an #else that avoids the
usage of non-ISO C extensions anyway so Clang do not throw any warnings or
compilers that don't define __GNUC__ still have IN6_* macros available. Again,
two implementations. Now I understand why was the original code in that form.

If the goal is to have a universal implementation it's no use to have it
wrapped in #ifdef __GNUC__. It just stops being universal.

Given the above, there is no use on exposing __in6_u.__u6_addr32 for the
purpose of fixing this bug, then.

Either we go back to patch #1, which keeps both implementations and just adds
the test for definition of __USE_GNU and __USE_MISC macros and does not break
strict compilers, or we remove the #ifdef __GNUC__ implementation altogether
and leave only the #else implementation unconditionally.

Sample of the #else implementation:
# define IN6_IS_ADDR_UNSPECIFIED(a) \
        (((const uint32_t *) (a))[0] == 0                                     \
         && ((const uint32_t *) (a))[1] == 0                                  \
         && ((const uint32_t *) (a))[2] == 0                                  \
         && ((const uint32_t *) (a))[3] == 0)

I will gladly attach a patch with just the #else implementation for your
review, but I still think the best bet to use the original patch.

Git commit aff2453d [1] (Dec 2011) shows that originally, only the #else
implementation existed but Ulrich Drepper added a GCC-specific implementation,
apparently to avoid warnings. Later, in commit a784e502 [2] (Jan 2012), he
changed the usage of __const to plain const, removing pre-ISO C
implementations.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #13 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Octavio Alvarez from comment #12)
> Thank you. Today I have learned about statement expressions and how they are
> GCC extensions.
> 
> If we add an #ifdef __GNUC__ we would also have to add an #else that avoids
> the usage of non-ISO C extensions anyway so Clang do not throw any warnings
> or compilers that don't define __GNUC__ still have IN6_* macros available.
> Again, two implementations. Now I understand why was the original code in
> that form.

That's correct.

> If the goal is to have a universal implementation it's no use to have it
> wrapped in #ifdef __GNUC__. It just stops being universal.

Agreed, but have a __GNUC__ version optimized for GNUC is good.

> Given the above, there is no use on exposing __in6_u.__u6_addr32 for the
> purpose of fixing this bug, then.

I don't follow.

> Either we go back to patch #1, which keeps both implementations and just
> adds the test for definition of __USE_GNU and __USE_MISC macros and does not
> break strict compilers, or we remove the #ifdef __GNUC__ implementation
> altogether and leave only the #else implementation unconditionally.

Not an option. Please leave the __GNUC__ implementation for optimally
supporting those using GNU extensions.

> Sample of the #else implementation:
> # define IN6_IS_ADDR_UNSPECIFIED(a) \
>         (((const uint32_t *) (a))[0] == 0                                   
> \
>          && ((const uint32_t *) (a))[1] == 0                                
> \
>          && ((const uint32_t *) (a))[2] == 0                                
> \
>          && ((const uint32_t *) (a))[3] == 0)
> 
> I will gladly attach a patch with just the #else implementation for your
> review, but I still think the best bet to use the original patch.
> 
> Git commit aff2453d [1] (Dec 2011) shows that originally, only the #else
> implementation existed but Ulrich Drepper added a GCC-specific
> implementation, apparently to avoid warnings. Later, in commit a784e502 [2]
> (Jan 2012), he changed the usage of __const to plain const, removing pre-ISO
> C implementations.

The fix I'll accept is:

- Unconditionally define __u6_* in the union.
- Fix all the macros.
- Test that.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[alvarezp at alvarezp dot ods.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #14 from Octavio Alvarez <alvarezp at alvarezp dot ods.org> ---
>> Either we go back to patch #1, which keeps both implementations and just
>> adds the test for definition of __USE_GNU and __USE_MISC macros and does not
>> break strict compilers, or we remove the #ifdef __GNUC__ implementation
>> altogether and leave only the #else implementation unconditionally.

> Not an option. Please leave the __GNUC__ implementation for optimally
> supporting those using GNU extensions.

Patch #1, the original one from Sebastien does exactly that.

> The fix I'll accept is:
> 
> - Unconditionally define __u6_* in the union.
> - Fix all the macros.
> - Test that.

By "fix all the macros", do you mean, rewrite them so they don't use statement
expressions?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[carlos at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #15 from Carlos O'Donell <carlos at redhat dot com> ---
On 02/04/2014 01:08 PM, alvarezp at alvarezp dot ods.org wrote:
>> Not an option. Please leave the __GNUC__ implementation for optimally
>> supporting those using GNU extensions.
> 
> Patch #1, the original one from Sebastien does exactly that.

Repost that patch to libc-alpha then for review.

>> The fix I'll accept is:
>>
>> - Unconditionally define __u6_* in the union.
>> - Fix all the macros.
>> - Test that.
> 
> By "fix all the macros", do you mean, rewrite them so they don't use statement
> expressions?

No, I mean fix them to use __u6_* in the union reference
so they operate independently of the public definitions
of the struct members.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #16 from Carlos O'Donell <carlos at redhat dot com> ---
Created attachment 7390
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7390&action=edit
Uconditionally define __u6_addr16 and __u6_addr32 ane use those in the macros.

This patch does what I explained is the correct solution.

It doesn't unconditionally define s6_addr16 or s6_addr32.

Instead we implement the macros using the internal types which are not
unconditionally defined.

Verified builds for x86-64, no regressions.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #17 from Carlos O'Donell <carlos at redhat dot com> ---
> Instead we implement the macros using the internal types which are not
> unconditionally defined.

s/not/now/g

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[alvarezp at alvarezp dot ods.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #18 from Octavio Alvarez <alvarezp at alvarezp dot ods.org> ---
I tested your patch against 24 combinations of gcc and clang invocations
relevant to lax and strict compilation. They all passed.

Thank you for pointing all that out. I had a misunderstanding of Andreas'
e-mail by assuming that the purpose of unconditionally defining _u6* was to
simplify into a single universal implementation.

I found another way that you may like. I'll attach a patch in a minute.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[alvarezp at alvarezp dot ods.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

Octavio Alvarez <alvarezp at alvarezp dot ods.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7388|0                           |1
        is obsolete|                            |

--- Comment #19 from Octavio Alvarez <alvarezp at alvarezp dot ods.org> ---
Created attachment 7391
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7391&action=edit
This patch proposes a single universal implementation, apparently consistent
with all the requirements stated in this bug report.


Characteristics:

* It exposes __in6_u.__u6_addr32[] unconditionally.
* It uses the exposed macros for internal implementation consistency.
* It keeps type casts in.
* It doesn't need non-ISO C extensions.
* It removes the need for #ifdef __GNUC__.
* It removes the alternate implementation altogether as it's unneeded now.
* Passes 24 combinations of gcc and clang compilation relevant to lax, strict
and standards compliance.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[joseph at codesourcery dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #20 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
I'd think the point of the __GNUC__ versions is to evaluate their argument 
only once.  (In the IN6_IS_ADDR_LINKLOCAL and IN6_IS_ADDR_SITELOCAL cases 
I don't see the point, but it seems to apply to the others.)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #21 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to joseph@codesourcery.com from comment #20)
> I'd think the point of the __GNUC__ versions is to evaluate their argument 
> only once.  (In the IN6_IS_ADDR_LINKLOCAL and IN6_IS_ADDR_SITELOCAL cases 
> I don't see the point, but it seems to apply to the others.)

I'm curious, what benefit do you derive from that?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[joseph at codesourcery dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #22 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
On Tue, 4 Feb 2014, carlos at redhat dot com wrote:

> https://sourceware.org/bugzilla/show_bug.cgi?id=16421
> 
> --- Comment #21 from Carlos O'Donell <carlos at redhat dot com> ---
> (In reply to joseph@codesourcery.com from comment #20)
> > I'd think the point of the __GNUC__ versions is to evaluate their argument 
> > only once.  (In the IN6_IS_ADDR_LINKLOCAL and IN6_IS_ADDR_SITELOCAL cases 
> > I don't see the point, but it seems to apply to the others.)
> 
> I'm curious, what benefit do you derive from that?

Generically it's seen as a good idea for macros to behave like functions 
as regards side-effects in arguments occurring exactly once.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[alvarezp at alvarezp dot ods.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #23 from Octavio Alvarez <alvarezp at alvarezp dot ods.org> ---
If that's the case, then Carlos' patch is the only one that complies with
everything. What would be the next step? Thanks.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[alvarezp at alvarezp dot ods.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

Octavio Alvarez <alvarezp at alvarezp dot ods.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7390|0                           |1
        is obsolete|                            |

--- Comment #24 from Octavio Alvarez <alvarezp at alvarezp dot ods.org> ---
Created attachment 7447
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7447&action=edit
Same as Carlos' patch, but updated to latest master.

This is an update to Carlos' patch to the latest master. Commit acd7f096d would
break this patch.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |security-

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug network/16421] IN6_IS_ADDR_UNSPECIFIED can use undefined s6_addr32

[alvarezp at alvarezp dot ods.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16421

--- Comment #25 from Octavio Alvarez <alvarezp at alvarezp dot ods.org> ---
What is preventing adoption for the fix of this bug?

-- 
You are receiving this mail because:
You are on the CC list for the bug.