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
> 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 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
* 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.
