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

[Langer, Christoph]

Hi,

I just pushed it after successfully running it through the hs submit repo: http://hg.openjdk.java.net/jdk/hs/rev/f654b37c58a1 

Thanks
Christoph

> -----Original Message-----
> From: Langer, Christoph
> Sent: Montag, 12. März 2018 14:24
> To: 'Chris Plummer' <chris.plummer at oracle.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
> 
> 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
> > >
> >