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

[David Holmes]

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.

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
>