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.
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.
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.
In my opinion the points are valid, because in opt builds there would be no
length check. 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.
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.
Best regards
Christoph
