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

Langer, Christoph christoph.langer at sap.com
Mon Mar 12 13:24:22 UTC 2018

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: >>> 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