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

[Chris Plummer]

On 3/5/18 3:31 PM, David Holmes wrote:
> On 6/03/2018 8:37 AM, Chris Plummer wrote:
>> 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);
>
> That fails to copy the '\0' - you need to keep "len + 1". Though again 
> why not just use strncpy.
>
Look closer. 1 is added to the MIN2 result. I agree with using strncpy.

Chris
> David
> -----
>
>> 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
>>