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

[Chris Plummer]

On 3/5/18 1:28 PM, Langer, Christoph wrote:
> Hi Chris,
>
>> Asserts imply something that is suppose to never happen, but that you
>> want to check for in debug builds to help uncover bugs. Given this,
>> either we have a bug (and someone can pass in a name that is too long),
>> or coverity is complaining about something that can never happen, or the
>> assert is invalid. So the potential fixes are:
>>
>> -Fix the problem up the call chain were the invalid string can be passed in.
>> -Tell coverity to clam up because having the string be too long is not
>> possible.
>> -Leave in your fix but remove the assert.
> I believe coverity has a valid point here for the case that strlen(name) == name_length_max or strlen(arg) == arg_length_max. In that case, memcpy would copy exactly the bytes for the strings without the terminating zero. And as zero initialization of c++ members is not guaranteed (as far as I know), the name_length_max + 1 byte or arg_legth_max + 1 could theoretically have nonzero values.
>
> Furthermore, I think in this case it makes sense to have an assertion because, as you state, in the debug builds you want to see any potential bug uncovered at the cost of a JVM exit. But in an opt build you want to be rather stable, even in case you get names and arguments passed that are too long. I don't want to go into the details of potential calling paths how that can happen, though... But even in case there are length violations in attach operation names or its arguments, the operations would most likely result in no success which is uncritical to a running VM.
>
> So wouldn't you agree that my change is fine as is?
>
> Submission-repo testing reported no errors.
>
> Best regards
> Christoph
>
Hi Christoph,

We don't assert things that we also explicitlydefend against.For 
example, the following defensive coding should not be accepted:

     // x is never suppose to be less then 0, but just in case it is, 
set it to 0
     assert(x >= 0);
     if (x < 0) x = 0;

Either it can't be less then zero and we assert this, or we accept the 
possibility that it could be less then zero and defend against it, but 
no assert in that case.

Given the following initial code (and ignoring what coverity might have 
to say about it):

  127     memcpy(_name, name, MIN2(len + 1, (size_t)name_length_max));

I would argue that the bug here is that it should be:

  127     memcpy(_name, name, MIN2(len, (size_t)name_length_max) + 1);

Maybe this would silence coverity, but it also is the correct thing to 
do. We weren't null terminating if len == name_length_max because we 
were only copying len bytes in that case, not len+1. But, this still has 
the issue of defending against something we also assert for, so the 
reality is the MIN2 part should be be needed at all. Does coverity 
complain if you get rid of it?

  127     memcpy(_name, name, len + 1);

thanks,

Chris