Hi Christoph,
On 3/8/18 3:38 AM, Langer, Christoph wrote:
Hi Chris,
I went back to the original code in coverity and checked what it complains
about.
This is the original code:
assert(strlen(name) <= name_length_max, "exceeds maximum name length");
strcpy(_name, name);
Coverity has various issues with this.
First, it generally considers strcpy as risky and doesn't like it at all. It
says:
secure_coding: [VERY RISKY]. Using "strcpy(char *, char const *)" can cause a
buffer overflow when done incorrectly. If the destination string of a strcpy() is not
large enough then anything might happen. Use strncpy() instead.
Generally speaking this is true, but not in this case since the caller
guards it with a length check.
Secondly, it doesn't accept the assert as length check and complains:
fixed_size_dest: You might overrun the 17-character fixed-size string
this->_name by copying name without checking the length.
Agreed that the assert is not a length check in product builds. However,
the only caller has a length check. Have you tried moving this length
check into set_name() and see if the problem goes away? Although I don't
suggest that as a fix. Just curious as to what the result would be.
Maybe it's coverity is concerned that the cmd could be changed between
the length check and the call to set_name.
BTW, I just realized I had been ignoring the set_arg() changes all this
time and focused on set_name(). So if any of the complaints are unique
to set_arg() please let me know.
And, 3rd, it considers the risk as elevated:
parameter_as_source: Note: This defect has an elevated risk because the source
argument is a parameter of the current function.
Is this a complaint about "name" being a source argument to strcpy(). If
so, I don't get this one. How are you going to copy "name" without
specifying it as an argument to something (strcpy, strncpy, memcpy,
etc). Besides, it is being passed to strcpy as a const argument. Makes
me wonder if adding const to the parameter declarations for both
set_name() and enqueue() would help.
In my opinion the points are valid, because in opt builds there would be no
length check.
But there is a length check in the caller. Does coverity not see checks
up the call chain?
Though we can see from the current code base that an overrun is virtually
impossible and coverity might also be smarter in terms of checking call paths
that lead to calls of set_name or set_arg, in the end there is no 100%
guarantee that this method would never be called with some bad parameter during
runtime, e.g. after someone changes code.
But if someone did that, we'd still have a bug, right?
I really think it would be easiest to go to my proposed patch. And it doesn't
come with much cost and the place probably isn't performance relevant.
I'm not worried about performance. To me it has more to do with taking
easily to read code and changing it into something that someone would
stare at for a bit before figuring out what it's doing, and then ask
"Why so complicated?". Coverity is suppose to help us make our code
better. I don't see that being the case here. If in the end your changes
are the simplest approach to quieting coverity, then I guess that's what
we should go with. However, I'm still not convinced we really fully why
converity is not happy with a strcpy that can be statically shown to be
safe. Is is a coverity bug? Is there a call path we are missing?
Something else that makes it hard for coverity to statically check this?
That's one reason I'd like to see what happens if a check is put
directly in set_name.
cheers,
Chris
Best regards
Christoph
