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

[Langer, Christoph]

Hi,

here is the final webrev for pushing: http://cr.openjdk.java.net/~clanger/webrevs/8199010.2/ I also did a little sorting in the include files (alphabetical order). The tests at SAP went fine and the coverity build was satisfied, too ��

Thanks in advance, Chris, for sponsoring.

Best regards
Christoph

> -----Original Message-----
> From: Chris Plummer [mailto:chris.plummer at oracle.com]
> Sent: Freitag, 9. März 2018 17:02
> To: Langer, Christoph <christoph.langer at sap.com>
> Cc: serviceability-dev at openjdk.java.net; Hotspot dev runtime <hotspot-
> runtime-dev at openjdk.java.net>; David Holmes
> <david.holmes at oracle.com>; Thomas Stüfe <thomas.stuefe at gmail.com>
> Subject: Re: RFR (XS): 8199010: attachListener.hpp: Fix potential null
> termination issue found by coverity scans
> 
> 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
> >
>