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
