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

Langer, Christoph christoph.langer at sap.com
Thu Mar 15 14:42:13 UTC 2018

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 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: > >>> fixedsizedest: 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 setname() 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 setname(), 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 setarg() changes all this > >> time and focused on setname(). So if any of the complaints are unique > >> to setarg() please let me know. > > No, nothing unique. > > > >>> And, 3rd, it considers the risk as elevated: > >>> parameterassource: 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 > >> setname() 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 setname. > > 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 setname() and setarg() 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 > > >

More information about the serviceability-dev mailing list