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
