Hi Kim: Thanks for the discussion, this makes more sense to me now.
> Kim Barrett <kim.barr...@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
