RFR(S): 8252407: Build failure with gcc-8+ and asan

[Eric Liu]

Hi Kim:

Thanks for the discussion, this makes more sense to me now. 

> Kim Barrett <kim.barrett at oracle.com> on 06 September 2020 19:35 wrote:
> 
> Can you be (very) specific about this.  Do all of these changes cause gcc10 to warn?  Or
> do only some of them.  If only some, specifically which ones? I have a conjecture about
> some of them (see below).  (I should probably try this myself; I know we have done some
> testing with gcc10, but I don’t remember where to find that devkit.)

I have tested 4 cases for those warnings:
a) Without my patch, without asan, gcc-8 and gcc-10 are OK.
b) Without my patch, with asan, gcc-8 has warned, gcc-10 is OK.
c) With my patch, without asan, gcc-8 and gcc-10 are OK.
d) With my patch, with asan, gcc-8 is OK, gcc-10 has warned.

Those warnings can be reported by both gcc-8 and gcc-10 only when asan enabled. Without
asan, no warning would be found even through some of them doesn't align with the documentation.

> ------------------------------------------------------------------------------
> src/java.base/unix/native/libnet/NetworkInterface.c
> 232     strncpy(searchName, name_utf, IFNAMESIZE - 1);
> 
> A better solution here would be to eliminate the strncpy entirely.
> 
> The strncpy is used to make a copy of a string so the copy can be
> searched for a colon, and if one is found, terminate the string there.
> 
> But there's no need to make such a copy.  The string being copied is
> the local temporary string name_utf.  We could instead use name_utf
> everywhere searchName is currently used, including clobbering the
> first colon to NUL.  We'd have to undo that before the later loop at
> line 249, but we have the information needed to do so.

Yes, this copy seems unnecessary. I was thinking whether it's a good way to find parent
by using strstr, so that we can not have to recover the zero. Will that be much slower?

> ------------------------------------------------------------------------------
> src/java.base/unix/native/libnet/NetworkInterface.c
> 1298     memset((char *)&if2, 0, sizeof(if2));
> 1299     strncpy(if2.ifr_name, name, sizeof(if2.ifr_name) - 1);
> 1300     if2.ifr_name[sizeof(if2.ifr_name) - 1] = 0;
> 
> (in getIndex)
> 
> So gcc8 does not warn about this, but gcc10 does? That would be a
> regression in gcc10 (and should be reported as such), because the
> documentation for -Wstringop-truncate is clear that the above is the
> proper idiom for avoiding the warning.

Yse, I followed the documentation and gcc-8 does not warn about this, but
gcc-10 does (both with asan enabled). 

> Regardless, the memset on 1298 is useless.  The code from before
> JDK-8250863 was the memset then the strncpy with
> sizeof(if2.ifr_name)-1 as the bound, which worked because the memset
> would have zeroed the last element.
> 
> The change for JDK-8250863 did not follow the documented idiom though.
> 
> It would be interesting to find out if removal of the memset changes
> anything.  It's barely conceivable that it's confusing the heuristics
> used to decide whether -Wstringop-truncation should trigger.  For
> example, the compiler might decide that the subsequent assignment of
> the last element is unnecessary because of the memset and optimize the
> assignment away, removing the idiomatic warning suppression.
> 
> If gcc10 still warns about the above after removing the memset then I
> see little recourse but to use PRAGMA_STRINGOP_TRUNCATION_IGNORED here.
>
> Similarly for the strncpy in getFlags:
> 1362     memset((char *)&if2, 0, sizeof(if2));
> 1363     strncpy(if2.ifr_name, ifname, sizeof(if2.ifr_name) - 1);
> 1364     if2.ifr_name[sizeof(if2.ifr_name) - 1] = 0;

After removing the memset, gcc-10(10.1.0) still warns about it, gcc-8 and gcc-9 also.

> ------------------------------------------------------------------------------
> src/java.base/unix/native/libnet/NetworkInterface.c
>  937     strncpy(name, if_name, IFNAMESIZE - 1);
>  938     name[IFNAMESIZE - 1] = '\0';
> 
> gcc10 presumably did not complain about the old version here, and this
> was not touched as part of JDK-8250863.  Does gcc10 complain about
> this new version?  If so, then I see little recoarse but to use
> PRAGMA_STRINGOP_TRUNCATION_IGNORED here, because a gcc10 warning here
> is contrary to the documentation.

Yes, gcc-10 would warn about this both before and after my patch if asan enabled.


> ------------------------------------------------------------------------------
> src/java.desktop/unix/native/libawt_xawt/awt/awt_InputMethod.c
> est/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/events/EM04/em04t001/em04t001.cpp
> 
> These changes look good, as they follow the documented idiom for
> suppressing -Wstringop-truncation. Does gcc10 warn after these changes?

Gcc-10 does'n warn about this changes.

> ------------------------------------------------------------------------------
> test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/libInheritedChannel.c
> (In Java_UnixDomainSocket_bind0)  
> 234     memset(&addr, 0, sizeof(addr));
> 235     addr.sun_family = AF_UNIX;
> 236     strncpy(addr.sun_path, nameUtf, length - 1);
> 237     addr.sun_path[length - 1] = '\0';
> 
> (In Java_UnixDomainSocket_connect0)
> 267     memset(&addr, 0, sizeof(addr));
> 268     addr.sun_family = AF_UNIX;
> 269     strncpy(addr.sun_path, nameUtf, length - 1);
> 270     addr.sun_path[length - 1] = '\0';
>
> These changes look good, as they follow the documented idiom for
> suppressing -Wstringop-truncation.
>
> Does gcc10 warn after these changes? 

Gcc-10 doesn't warn about these changes.

I'm working on a new patch that to make both gcc-8 and gcc-10 happy.

Thanks,
Eric