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
