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

[Chris Plummer]

On 3/6/18 4:26 AM, David Holmes wrote:
> 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?
 From the man page:

      The stpncpy() and strncpy() functions copy at most len characters 
from src into dst.  If src is less than len
      characters long, the remainder of dst is filled with `\0' 
characters.  Otherwise, dst is not terminated.

>
> 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.
Why don't we do a restart and look at the original problem whose fix 
introduced the current issue. Here's the original code:

     assert(strlen(name) <= name_length_max, "exceeds maximum name length");
     strcpy(_name, name);

And here's how set_name is used:

class AttachOperation: public CHeapObj<mtInternal> {
   enum {
     name_length_max = 16,       // maximum length of  name
   };
   char _name[name_length_max+1];
}

   if (strlen(cmd) > AttachOperation::name_length_max) return 
ATTACH_ERROR_ILLEGALARG;
   op->set_name(cmd);

I don't see why coverity would complain about this. It can statically 
see that there will be no buffer overflow. Does it think there are other 
potential callers of set_name() for which no size check is made? I can't 
find any. The only clue from JDK-8140482 is:

attachListener.hpp:
Do strncpy to not overflow buffer. Don't write more chars than before.

David, you had pointed that the strcpy complained seemed erroneous 
during the review for JDK-8140482. The final word from Goetz was:

> I agree that I can not find another possible issue
> with the strcpy.
> Still I think it's better to have the strncpy, as it would have
> protected against the bug in attachListener_windows.cpp.
> But if you insist I'll just remove it.
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-November/016363.html

Dmitry Samersoff later added:
> It might be better to calculate strlen(name) once, than use memcpy.
So we started with what appears to be an invalid and unexplained 
complaint by coverity about strcpy, changed it to strncpy to appease 
coverity, changed it to memcpy to avoid two calls to strlen, and now 
want to fix a different complaint by coverity on that solution.

I say go back to strcpy and see if/why coverity is still complaining.

thanks,

Chris
>
> Thanks,
> David
>>
>> What shall I do now?
>>
>> Best regards
>> Christoph
>>