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

[Chris Plummer]

On 3/9/18 4:50 AM, Langer, Christoph wrote:
> Hi Chris,
>
>>> 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.
> When doing a length check in set_name(), coverity would be pleased. But still we'd have to handle length violations by either guaranteeing or returning some error return code, or quietly truncating. But you say you don't suggest it as fix anyway...
>
>> 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.
> No, nothing unique.
>
>>> 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.
> I think coverity just considers this finding as elevated because the input data isn't something static from inside the method but comes in as argument.
>
>>> 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?
> Obviously not.
>
>>> 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.
> OK, so let me summarize:
> The code as it is right now has a little issue - which isn't obvious at a quick glance by the way.
> It can be fixed like I suggested. This would add two lines of code at each place and one can argue about how easy it is to understand. To me it seems as understandable as it was before - but I'm probably a bit concerned here.
In terms of readability, I was referring back to the original code that 
just had the strlen. It was the original coverity fix to that code that 
introduced readability issue. You aren't really doing much to make it 
less readable.
> I can suggest an alternative which might be easier to read: http://cr.openjdk.java.net/~clanger/webrevs/8199010.1/ It comes at the cost of 2 calls to strlen() in dbg builds but it has one line of code less and might be more straightforward to understand.
> All larger refactoring of set_name() and set_arg() is beyond the scope of my change.
I like this version better, although it doesn't change my opinion that 
this is still all jumping through hoops to get coverity to stop 
complaining about something that is perfectly fine.
>
> Now I'd really like if you could accept one of my 2 proposals, given that also Thomas and David think it's ok. I want to get this done now. �� Maybe you can even sponsor it...
Yeah, I'm ok with the change. I've said my peace and don't just want to 
get in the way of a simple fix. Yes, I can also sponsor it for you.

cheers,

Chris
>
> Thanks & Best regards
> Christoph
>