Thanks, David. A colleague just told me that a guarantee would also quiesce Coverity. So that could really be an option then. Let's wait for Chris' opinion...
> -----Original Message----- > From: David Holmes [mailto:david.hol...@oracle.com] > Sent: Dienstag, 6. März 2018 13:26 > To: Langer, Christoph <christoph.lan...@sap.com>; Chris Plummer > <chris.plum...@oracle.com>; Thomas Stüfe <thomas.stu...@gmail.com> > Cc: serviceability-dev@openjdk.java.net; Hotspot dev runtime <hotspot- > runtime-...@openjdk.java.net> > Subject: Re: RFR (XS): 8199010: attachListener.hpp: Fix potential null > termination issue found by coverity scans > > On 6/03/2018 6:50 PM, Langer, Christoph wrote: > > Hi Chris, David and Thomas, > > > > I took a closer look now, too. Funny that the original change was > contributed by my colleagues because of coverity and that they didn't do it > completely right. 😉 As a code comment in our attachListener.hpp suggests, > the '0' termination to please coverity was added far earlier than JDK-8140482 > was done. > > > > So, yes, in fact the input to the "set_name" and "set_arg" methods should > never exceed the maximum length values as per the current code in the > OpenJDK. These methods are called from the various platform specific > attachListener_<os>.cpp files. And in each of these places the length is > already checked and violations get handled. So with the assertion we merely > guard against new code that doesn't do checking which can potentially come > in. > > > > So one can argue that the assertions are enough here and we can just do > strcpy. In that case I would even support Thomas' suggestion to change the > assertion into a guarantee as the input coming in from new code is not > necessarily static but can be user input (who knows). And we should also > turn the knob here to quiesce coverity since it is obviously not considering > the possible call paths and the checks in them. > > > > But on the other hand, one could be as conservative as it is now - I guess > > it > doesn't bear too much of cost and this place of code is not performance > critical. That means do the assertion in dbg builds and for opt effectively > do a > checked, truncating copy of the input data but avoiding JVM crashes or other > errors due to unterminated strings. > > > > I personally tend to do the second - but fine if I get overruled. > > > > But, if we do the second, I'm still for memcpy as strncpy would do zero > padding of the buffer which is not necessary and we have to write a > terminating 0 as well to handle the case that inputlength > name_len_max > (the case which should not happen but we want to protect against). That > would mean my change stays as it is. > > I don't know why strncpy would do zero padding? > > Personally I view this code as follows: > > - it is guaranteed that name length can not exceed the expected maximum > due to existing checks > - the assert guards against new code that might add an unchecked path or > a new command name that is longer than current max and doesn't update > the max > > With that in mind then a simple strncpy of len+1 fully suffices. > > However that doesn't address the coverity issue (and possibly other > checking tools). And given this code was already appeasing coverity, I > vote for just accepting Christoph's patch. > > Thanks, > David > > > > What shall I do now? > > > > Best regards > > Christoph > >
