Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
Hi, Thanks for looking at this. For gcc-10, it's hard to make 'strncpy' all right with asan enabled (approaches we talked previous don't work). I'm trying to find a better way to avoid using compile pragma. I suppose it would be better to use 'memcpy' to replace 'strncpy'. Thanks, Eric
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
On 24/09/2020 09:59, Kim Barrett wrote: If possible, my preference would be to avoid the pragma cruft and write the code in such a way that gcc8/9 without asan doesn't warn, and gcc10 doesn't warn with or without asan. I've kind of lost track in the discussion of all the variants whether that's actually feasible. I agree, the program cruft in NetworkInterface.c is unmaintainable and not the right place for it. -Alan.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 23, 2020, at 2:10 AM, Eric Liu wrote: > > Hi Kim, > > Sorry for the delay. > > This patch removes a redundant string copy in NetworkInterface.c to avoid > string-truncation > warning. Other warnings we talked before, which are unable to completely fix > in different version > of gcc, I have to use pragma to suppress them as a workaround. > > This patch now could compile with gcc-7, gcc-8, gcc-9, gcc-10 both with or > without asan. > > [TESTS] > Jtreg: hotspot::hotspot_all_no_apps, jdk::jdk_core and langtools::tier1. > No new failure found. > > http://cr.openjdk.java.net/~qfeng/ericliu/jdk/stringop_trunc/webrev.01/ > https://bugs.openjdk.java.net/browse/JDK-8252407 If possible, my preference would be to avoid the pragma cruft and write the code in such a way that gcc8/9 without asan doesn't warn, and gcc10 doesn't warn with or without asan. I've kind of lost track in the discussion of all the variants whether that's actually feasible.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
Hi Kim, Sorry for the delay. This patch removes a redundant string copy in NetworkInterface.c to avoid string-truncation warning. Other warnings we talked before, which are unable to completely fix in different version of gcc, I have to use pragma to suppress them as a workaround. This patch now could compile with gcc-7, gcc-8, gcc-9, gcc-10 both with or without asan. [TESTS] Jtreg: hotspot::hotspot_all_no_apps, jdk::jdk_core and langtools::tier1. No new failure found. http://cr.openjdk.java.net/~qfeng/ericliu/jdk/stringop_trunc/webrev.01/ https://bugs.openjdk.java.net/browse/JDK-8252407 Thanks, Eric
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
Hi Kim, > Kim Barrett on Sent: 08 September 2020 20:28 >> On Sep 7, 2020, at 10:56 AM, Eric Liu wrote: >> 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. > That’s *very* strange, since I get warnings in NetworkInterface getIndex and > getFlags > with gcc10.2 + your patch. Sorry for the mistake, gcc-10 exactly warn about my patch. >> 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 >>> 1298 memset((char *), 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 *), 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. > I’m confused. Above you said that with your patch + asan gcc-8 did not warn. > Maybe what you are saying is that *just* removing the memset, and not applying > your patch, for this case with asan warns with all of gcc-8,9,10? Sorry for my bad expression. Actually removing memset could not make things better. My patch + asan would annoy gcc-10, removing memset changed nothing. For gcc-8 and gcc-9 part, without my patch they would warn about with asan, and removing memset can not fix them as well. Thanks, Eric
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 7, 2020, at 5:55 AM, Florian Weimer wrote:
>
> * Kim Barrett:
>
>> And strlen is not even necessarily the best solution, as it likely
>> introduces an additional otherwise unnecessary string traversal. For
>> example, getFlags could be changed to reject an overly long ifname,
>> without using strlen, thusly:
>>
>>strncpy(if2.ifr_name, ifname, sizeof(if2.ifr_name));
>>if (if2.ifr_name[sizeof(if2.ifr_name) - 1] != '\0') {
>>return -1;
>>}
>>
>> Unfortunately, gcc10 -Wstringop-truncation whines about this entirely
>> reasonable code.
>
> Thanks, I filed this as:
> >
Thanks. Though it looks like the response is just “don’t use strncpy”.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 7, 2020, at 10:56 AM, Eric Liu wrote: > 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. That’s *very* strange, since I get warnings in NetworkInterface getIndex and getFlags with gcc10.2 + your patch. > 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 >> 1298 memset((char *), 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 *), 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. I’m confused. Above you said that with your patch + asan gcc-8 did not warn. Maybe what you are saying is that *just* removing the memset, and not applying your patch, for this case with asan warns with all of gcc-8,9,10? > I'm working on a new patch that to make both gcc-8 and gcc-10 happy. Good luck! I think I’m coming around to Florian’s “never use strncpy” position, not so much because it’s the wrong function to use (though sometimes it probably is, or is being used wrongly), but because fighting with -Wstringop-truncation is just not worth the aggravation.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
Hi Kim: Thanks for the discussion, this makes more sense to me now. > Kim Barrett 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 *), 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 *), 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
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
* Kim Barrett:
> And strlen is not even necessarily the best solution, as it likely
> introduces an additional otherwise unnecessary string traversal. For
> example, getFlags could be changed to reject an overly long ifname,
> without using strlen, thusly:
>
> strncpy(if2.ifr_name, ifname, sizeof(if2.ifr_name));
> if (if2.ifr_name[sizeof(if2.ifr_name) - 1] != '\0') {
> return -1;
> }
>
> Unfortunately, gcc10 -Wstringop-truncation whines about this entirely
> reasonable code.
Thanks, I filed this as:
Florian
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 6, 2020, at 9:51 PM, Kim Barrett wrote: > Oh, good grief! This file contains 3 identical copies of getMTU and > getFlags, one each for linux, AIX, and BSD. And getIndex is kind of a > mess. If changing any of these for linux, probably similar changes > ought to be applied to the other copies to keep things in sync. Is there any reason the linux version of getIndex couldn’t be changed to use if_nametoindex (like the AIX version)? That would eliminate one of the cases that is causing problems.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 6, 2020, at 1:03 PM, Florian Weimer wrote:
> There is no reason to use strncpy. At least on Linux, the struct field
> needs to be null-terminated, and you need to compute the length for the
> length check. So you might as well use memcpy with the length plus one
> (to copy the null terminator). You can keep using strncpy, and the
> warning should be gone (because GCC will recognize a dominating strlen
> check), but it's not necessary.
>
> On current GNU/Linux, the most common structs now have the appropriate
> annotations, so you get the strncpy warnings only in cases where there
> is an actual truncation bug.
[Keep in mind that gcc on recent GNU/Linux is not the only platform
this code base needs to worry about. Though it is the one that is
driving us a bit batty with its varying warnings here.]
I'm not sure whether you are suggesting one should never use strncpy
anywhere, or whether there is some specific place(s) from this webrev
you are suggesting it should be avoided?
I agree there are some uses of strncpy that look at least somewhat
suspect. Without further analysis it's hard to know whether there is
really a problem with truncation (can it occur at all, and is it a
problem if it does). Figuring that out seems somewhat beyond the
scope of what this change is trying to do. Please file bugs about
problematic uses. (A bug of the form "don't use strncpy" might not
get quick action.)
And strlen is not even necessarily the best solution, as it likely
introduces an additional otherwise unnecessary string traversal. For
example, getFlags could be changed to reject an overly long ifname,
without using strlen, thusly:
strncpy(if2.ifr_name, ifname, sizeof(if2.ifr_name));
if (if2.ifr_name[sizeof(if2.ifr_name) - 1] != '\0') {
return -1;
}
Unfortunately, gcc10 -Wstringop-truncation whines about this entirely
reasonable code.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 6, 2020, at 7:35 AM, Kim Barrett wrote: > src/java.base/unix/native/libnet/NetworkInterface.c > 1298 memset((char *), 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. > > 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. I found a gcc10.2 devkit that I could take for a spin with Eric's patches. I got -Wstringop-truncation warnings in NetworkInterface.c, in getIndex and getFlags. Removing the immediately preceeding memset removed those warnings. (Note that in these two cases the memset is otherwise completely superfluous, both before and after Eric's changes.) Strangely, there are other places in this file where a strncpy is preceeded by a memcpy and there is no assignment of the last element to NUL, but which no version of gcc seems to complain about. Examples include getMacAddress, getMTU, enumIPv6Interfaces. Oh, good grief! This file contains 3 identical copies of getMTU and getFlags, one each for linux, AIX, and BSD. And getIndex is kind of a mess. If changing any of these for linux, probably similar changes ought to be applied to the other copies to keep things in sync. After removing the superfluous memsets from getIndex and getFlags, my test build with gcc10.2 completed successfully with Eric's patches.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 6, 2020, at 7:35 AM, Kim Barrett wrote: > src/hotspot/share/compiler/compileBroker.hpp > 64 PRAGMA_DIAG_PUSH > 65 PRAGMA_STRINGOP_TRUNCATION_IGNORED > 66 // This code can incorrectly cause a "stringop-truncation" warning > with gcc > 67 strncpy(_current_method, method, (size_t)cmname_buffer_length-1); > 68 PRAGMA_DIAG_POP > 69 _current_method[cmname_buffer_length-1] = '\0'; > > I think I'd prefer the PRAGMA_DIAG_POP moved one line further down, so > that the push/pop surrounds the entire idiomactic usage that is > supposed to prevent the warning. This seems to be a gcc8 bug. > > gcc10 doesn't warn about this (and shouldn't). It would be > interesting to know if it too warns with --enable-asan. I keep forgetting. This is the one where we think the cast on line 67 might be confusing gcc8, and if it were eliminated we might not need the PRAGMA dance here after all.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
* Kim Barrett: >> On Sep 4, 2020, at 7:50 AM, Florian Weimer wrote: >> >> * Daniel Fuchs: >> >>> Hi, >>> >>> On 02/09/2020 08:19, Florian Weimer wrote: At least one of the bugs was in theory user-visible: the network interface code would return data for an interface that does not actually exist on the system. >>> >>> WRT NetworkInterface.c, I might support using `strnlen` to check >>> the length before hand, if that solves both cases (gcc8 and gcc10). >>> I'm always a bit nervous of changing the behavior in this library >>> as it's hard to verify that no regression is introduced. >> >> I think you should use strlen. If the string is longer than the buffer >> sent to the kernel, it cannot match an existing interface because all >> the names are shorter. So some sort of “not found” error needs to >> reported. > > That may be, but I think doing so probably won't do anything to > address the -Wstringop-truncation warnings. There is no reason to use strncpy. At least on Linux, the struct field needs to be null-terminated, and you need to compute the length for the length check. So you might as well use memcpy with the length plus one (to copy the null terminator). You can keep using strncpy, and the warning should be gone (because GCC will recognize a dominating strlen check), but it's not necessary. On current GNU/Linux, the most common structs now have the appropriate annotations, so you get the strncpy warnings only in cases where there is an actual truncation bug. Thanks, Florian
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 1, 2020, at 9:59 AM, Eric Liu wrote: > I just tested this patch by GCC (10.1.0) and it would really re-trigger those > warnings :( > I have not noticed the history before, but it's really hard to imagine that > GCC would > have different behaviors. 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.) -- 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. -- src/java.base/unix/native/libnet/NetworkInterface.c 1298 memset((char *), 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. 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 *), 0, sizeof(if2)); 1363 strncpy(if2.ifr_name, ifname, sizeof(if2.ifr_name) - 1); 1364 if2.ifr_name[sizeof(if2.ifr_name) - 1] = 0; -- 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. -- src/hotspot/share/compiler/compileBroker.hpp 64 PRAGMA_DIAG_PUSH 65 PRAGMA_STRINGOP_TRUNCATION_IGNORED 66 // This code can incorrectly cause a "stringop-truncation" warning with gcc 67 strncpy(_current_method, method, (size_t)cmname_buffer_length-1); 68 PRAGMA_DIAG_POP 69 _current_method[cmname_buffer_length-1] = '\0'; I think I'd prefer the PRAGMA_DIAG_POP moved one line further down, so that the push/pop surrounds the entire idiomactic usage that is supposed to prevent the warning. This seems to be a gcc8 bug. gcc10 doesn't warn about this (and shouldn't). It would be interesting to know if it too warns with --enable-asan. -- 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? -- test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/libInheritedChannel.c (In Java_UnixDomainSocket_bind0) 234 memset(, 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(, 0, sizeof(addr)); 268
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 4, 2020, at 7:50 AM, Florian Weimer wrote: > > * Daniel Fuchs: > >> Hi, >> >> On 02/09/2020 08:19, Florian Weimer wrote: >>> At least one of the bugs was in theory user-visible: the network >>> interface code would return data for an interface that does not actually >>> exist on the system. >> >> WRT NetworkInterface.c, I might support using `strnlen` to check >> the length before hand, if that solves both cases (gcc8 and gcc10). >> I'm always a bit nervous of changing the behavior in this library >> as it's hard to verify that no regression is introduced. > > I think you should use strlen. If the string is longer than the buffer > sent to the kernel, it cannot match an existing interface because all > the names are shorter. So some sort of “not found” error needs to > reported. That may be, but I think doing so probably won't do anything to address the -Wstringop-truncation warnings. > (I assume that it's actually a bug that you can look up a network > interface by a name that merely shares the same prefix with an actual > interface on the system.) > > Thanks, > Florian
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
* Daniel Fuchs: > Hi, > > On 02/09/2020 08:19, Florian Weimer wrote: >> At least one of the bugs was in theory user-visible: the network >> interface code would return data for an interface that does not actually >> exist on the system. > > WRT NetworkInterface.c, I might support using `strnlen` to check > the length before hand, if that solves both cases (gcc8 and gcc10). > I'm always a bit nervous of changing the behavior in this library > as it's hard to verify that no regression is introduced. I think you should use strlen. If the string is longer than the buffer sent to the kernel, it cannot match an existing interface because all the names are shorter. So some sort of “not found” error needs to reported. (I assume that it's actually a bug that you can look up a network interface by a name that merely shares the same prefix with an actual interface on the system.) Thanks, Florian
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
On 2020-09-02 09:50, Kim Barrett wrote: On Sep 2, 2020, at 2:39 AM, Magnus Ihse Bursie wrote: On 2020-09-01 11:46, Kim Barrett wrote: I really hate -Wstringop-truncation. It's been a constant source of churn for us ever since it appeared. The changes being made to getIndex and getFlags (NetworkInterface.c) are modifying lines that were changed very recently to deal with such warnings from gcc10. I'm worried that these new changes will re-trigger warnings from gcc10 (though this change isn't a revert; the gcc10 warning was justifiable). I think it should be okay, but there’s some risk here. Maybe we should have a common library for all native code where we supply our own string operation functions? It will then be much easier to make sure the implementation passes different compiler versions, and that we provide sane semantics (which isn't really the case with the original C library functions; hence all this warning churning). For the recurring problems with strncpy, there’s already this: https://bugs.openjdk.java.net/browse/JDK-8232187 Nobody’s picked it up yet. Yes, but that's hotspot only. The other JDK libraries would not be able to use it. (And as I said, there are other, already existing functions, that ideally should be shared between hotspot and the rest of the libraries). /Magnus
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
Hi, On 02/09/2020 08:19, Florian Weimer wrote: At least one of the bugs was in theory user-visible: the network interface code would return data for an interface that does not actually exist on the system. WRT NetworkInterface.c, I might support using `strnlen` to check the length before hand, if that solves both cases (gcc8 and gcc10). I'm always a bit nervous of changing the behavior in this library as it's hard to verify that no regression is introduced. best regards, -- daniel
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 2, 2020, at 3:19 AM, Florian Weimer wrote: > > * Magnus Ihse Bursie: > >> Maybe we should have a common library for all native code where we >> supply our own string operation functions? It will then be much easier >> to make sure the implementation passes different compiler versions, >> and that we provide sane semantics (which isn't really the case with >> the original C library functions; hence all this warning churning). > > When I looked at this (sorry that I never sent a patch), pretty much all > uses of strncpy were actually bugs: The code should check the actual > string length using strlen, report an error if it would be truncated, > and then use memcpy with the length already computed, plus one. > > In other words, the strncpy warnings are correct, and there is only > churn in the sense that GCC gets smarter at discovering bugs. Having been involved in reviews of a lot of these warnings, I think that’s seriously overstating the cases where there have been actual bugs. There have been some actual bugs found, but there have been a lot of definite false positives. Particularly from -Wstringop-truncation.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 2, 2020, at 2:39 AM, Magnus Ihse Bursie > wrote: > > On 2020-09-01 11:46, Kim Barrett wrote: >> I really hate -Wstringop-truncation. It's been a constant source of churn >> for us ever since it appeared. The changes being made to getIndex and >> getFlags (NetworkInterface.c) are modifying lines that were changed very >> recently to deal with such warnings from gcc10. I'm worried that these new >> changes will re-trigger warnings from gcc10 (though this change isn't a >> revert; the gcc10 warning was justifiable). I think it should be okay, but >> there’s some risk here. > Maybe we should have a common library for all native code where we supply our > own string operation functions? It will then be much easier to make sure the > implementation passes different compiler versions, and that we provide sane > semantics (which isn't really the case with the original C library > functions; hence all this warning churning). For the recurring problems with strncpy, there’s already this: https://bugs.openjdk.java.net/browse/JDK-8232187 Nobody’s picked it up yet.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
* Magnus Ihse Bursie: > Maybe we should have a common library for all native code where we > supply our own string operation functions? It will then be much easier > to make sure the implementation passes different compiler versions, > and that we provide sane semantics (which isn't really the case with > the original C library functions; hence all this warning churning). When I looked at this (sorry that I never sent a patch), pretty much all uses of strncpy were actually bugs: The code should check the actual string length using strlen, report an error if it would be truncated, and then use memcpy with the length already computed, plus one. In other words, the strncpy warnings are correct, and there is only churn in the sense that GCC gets smarter at discovering bugs. At least one of the bugs was in theory user-visible: the network interface code would return data for an interface that does not actually exist on the system. Thanks, Florian
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
On 2020-09-01 11:46, Kim Barrett wrote: On Sep 1, 2020, at 4:01 AM, Eric Liu wrote: Hi all, Please review this simple change to fix some compile warnings. The newer gcc (gcc-8 or higher) would warn for calls to bounded string manipulation functions such as 'strncpy' that may either truncate the copied string or leave the destination unchanged. This patch fixed stringop-truncation warnings reported by gcc, some of them only appear when compiled with "--enable-asan". [TESTS] Jtreg: hotspot::hotspot_all_no_apps, jdk::jdk_core and langtools::tier1. No new failure found. http://cr.openjdk.java.net/~qfeng/ericliu/jdk/stringop_trunc/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8252407 Thanks, Eric I really hate -Wstringop-truncation. It's been a constant source of churn for us ever since it appeared. The changes being made to getIndex and getFlags (NetworkInterface.c) are modifying lines that were changed very recently to deal with such warnings from gcc10. I'm worried that these new changes will re-trigger warnings from gcc10 (though this change isn't a revert; the gcc10 warning was justifiable). I think it should be okay, but there’s some risk here. Maybe we should have a common library for all native code where we supply our own string operation functions? It will then be much easier to make sure the implementation passes different compiler versions, and that we provide sane semantics (which isn't really the case with the original C library functions; hence all this warning churning). There have been other problem areas before, where a common library (static or dynamic) would have helped. Perhaps it's time to go ahead and create one... /Magnus Changes look good, subject to that caveat. I think these changes conform better to the documented description of the warning than did the recent NetworkInterface.c change mentioned above, so I’m hopeful that we’re not in a warning cycle here. But it would be good to have someone test these changes against gcc10.x.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
Hi Daniel, Kim, Thanks for your review. > Kim Barrett on Tue Sep 1 09:46:26 UTC 2020 > > Changes look good, subject to that caveat. I think these changes conform > better to the documented description of the warning than did the recent > NetworkInterface.c change mentioned above, so I’m hopeful that we’re not > in a warning cycle here. But it would be good to have someone test these > changes against gcc10.x. I just tested this patch by GCC (10.1.0) and it would really re-trigger those warnings :( I have not noticed the history before, but it's really hard to imagine that GCC would have different behaviors. >Daniel Fuchs on Tue Sep 1 11:59:17 UTC 2020 > > Thanks for reminding me of these changes. > Indeed, the changes proposed to NetworkInteface.c - though > not incorrect - may well re-trigger this gcc10 warning [1]. > So now I don't think this should go through unless it's verified > that it doesn't cause further issues down the road. Do you think it's a good idea to suppress those warnings by pragma? As we already know the code are actually correct, and that would make GCC happy both for 9 and 10. Thanks, Eric
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
Hi Eric, Kim, On 01/09/2020 10:46, Kim Barrett wrote: he changes being made to getIndex and getFlags (NetworkInterface.c) are modifying lines that were changed very recently to deal with such warnings from gcc10. I'm worried that these new changes will re-trigger warnings from gcc10 (though this change isn't a revert; the gcc10 warning was justifiable). I think it should be okay, but there’s some risk here. Thanks for reminding me of these changes. Indeed, the changes proposed to NetworkInteface.c - though not incorrect - may well re-trigger this gcc10 warning [1]. So now I don't think this should go through unless it's verified that it doesn't cause further issues down the road. [1] https://bugs.openjdk.java.net/browse/JDK-8250863 best regards, -- daniel
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 1, 2020, at 5:46 AM, Kim Barrett wrote: > >> On Sep 1, 2020, at 4:01 AM, Eric Liu wrote: >> >> Hi all, >> >> Please review this simple change to fix some compile warnings. >> >> The newer gcc (gcc-8 or higher) would warn for calls to bounded string >> manipulation functions such as 'strncpy' that may either truncate the >> copied string or leave the destination unchanged. >> >> This patch fixed stringop-truncation warnings reported by gcc, some of >> them only appear when compiled with "--enable-asan". >> >> [TESTS] >> Jtreg: hotspot::hotspot_all_no_apps, jdk::jdk_core and langtools::tier1. >> No new failure found. >> >> http://cr.openjdk.java.net/~qfeng/ericliu/jdk/stringop_trunc/webrev.00/ >> https://bugs.openjdk.java.net/browse/JDK-8252407 >> >> Thanks, >> Eric > > I really hate -Wstringop-truncation. It's been a constant source of churn > for us ever since it appeared. The changes being made to getIndex and > getFlags (NetworkInterface.c) are modifying lines that were changed very > recently to deal with such warnings from gcc10. I'm worried that these new > changes will re-trigger warnings from gcc10 (though this change isn't a > revert; the gcc10 warning was justifiable). I think it should be okay, but > there’s some risk here. > > Changes look good, subject to that caveat. Apparently I forgot about the discussion of the casted enum, so belay that. > I think these changes conform > better to the documented description of the warning than did the recent > NetworkInterface.c change mentioned above, so I’m hopeful that we’re not > in a warning cycle here. But it would be good to have someone test these > changes against gcc10.x.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 1, 2020, at 4:01 AM, Eric Liu wrote: > > Hi all, > > Please review this simple change to fix some compile warnings. > > The newer gcc (gcc-8 or higher) would warn for calls to bounded string > manipulation functions such as 'strncpy' that may either truncate the > copied string or leave the destination unchanged. > > This patch fixed stringop-truncation warnings reported by gcc, some of > them only appear when compiled with "--enable-asan". > > [TESTS] > Jtreg: hotspot::hotspot_all_no_apps, jdk::jdk_core and langtools::tier1. > No new failure found. > > http://cr.openjdk.java.net/~qfeng/ericliu/jdk/stringop_trunc/webrev.00/ > https://bugs.openjdk.java.net/browse/JDK-8252407 > > Thanks, > Eric I really hate -Wstringop-truncation. It's been a constant source of churn for us ever since it appeared. The changes being made to getIndex and getFlags (NetworkInterface.c) are modifying lines that were changed very recently to deal with such warnings from gcc10. I'm worried that these new changes will re-trigger warnings from gcc10 (though this change isn't a revert; the gcc10 warning was justifiable). I think it should be okay, but there’s some risk here. Changes look good, subject to that caveat. I think these changes conform better to the documented description of the warning than did the recent NetworkInterface.c change mentioned above, so I’m hopeful that we’re not in a warning cycle here. But it would be good to have someone test these changes against gcc10.x.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
Hi Eric, The changes to NetworkInterface.c look good to me. best regards, -- daniel On 01/09/2020 09:01, Eric Liu wrote: Hi all, Please review this simple change to fix some compile warnings. The newer gcc (gcc-8 or higher) would warn for calls to bounded string manipulation functions such as 'strncpy' that may either truncate the copied string or leave the destination unchanged. This patch fixed stringop-truncation warnings reported by gcc, some of them only appear when compiled with "--enable-asan". [TESTS] Jtreg: hotspot::hotspot_all_no_apps, jdk::jdk_core and langtools::tier1. No new failure found. http://cr.openjdk.java.net/~qfeng/ericliu/jdk/stringop_trunc/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8252407 Thanks, Eric
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
Hi all, Please review this simple change to fix some compile warnings. The newer gcc (gcc-8 or higher) would warn for calls to bounded string manipulation functions such as 'strncpy' that may either truncate the copied string or leave the destination unchanged. This patch fixed stringop-truncation warnings reported by gcc, some of them only appear when compiled with "--enable-asan". [TESTS] Jtreg: hotspot::hotspot_all_no_apps, jdk::jdk_core and langtools::tier1. No new failure found. http://cr.openjdk.java.net/~qfeng/ericliu/jdk/stringop_trunc/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8252407 Thanks, Eric
