RFR (XS): 8199010: attachListener.hpp: Fix potential null termination issue found by coverity scans
chris.plummer at oracle.com
Mon Mar 5 23:53:21 UTC 2018
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
>>>> 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
>>>> -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
>> 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.
>> 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);
More information about the hotspot-runtime-dev