RFR (XS): 8199010: attachListener.hpp: Fix potential null termination issue found by coverity scans

[Chris Plummer]

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
>