RFR (XS): 8199010: attachListener.hpp: Fix potential null termination issue found by coverity scans
Langer, Christoph
christoph.langer at sap.com
Thu Mar 8 11:38:34 UTC 2018
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
More information about the serviceability-dev
mailing list