RFR (XS): 8199010: attachListener.hpp: Fix potential null termination issue found by coverity scans

[Langer, Christoph]

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.holmes at oracle.com]
> Sent: Dienstag, 6. März 2018 13:26
> To: Langer, Christoph <christoph.langer at sap.com>; Chris Plummer
> <chris.plummer at oracle.com>; Thomas Stüfe <thomas.stuefe at gmail.com>
> Cc: serviceability-dev at openjdk.java.net; Hotspot dev runtime <hotspot-
> runtime-dev at 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
> >