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.plum...@oracle.com> > Cc: serviceability-dev@openjdk.java.net; Hotspot dev runtime <hotspot- > runtime-...@openjdk.java.net>; David Holmes > <david.hol...@oracle.com>; Thomas Stüfe <thomas.stu...@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.plum...@oracle.com] > > Sent: Freitag, 9. März 2018 17:02 > > To: Langer, Christoph <christoph.lan...@sap.com> > > Cc: serviceability-dev@openjdk.java.net; Hotspot dev runtime <hotspot- > > runtime-...@openjdk.java.net>; David Holmes > > <david.hol...@oracle.com>; Thomas Stüfe <thomas.stu...@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 > > > > >
