Hi Christoph, thanks for doing this change. I would appreciate if the coverity findings get into the codebase, at minimum it simplifies repeated runs. I’m not sure about the syntax cleanups, especially invoker.c, this seems too trivial to touch the file.
Thanks for explaining inStream.c, I think it would help documenting the number or computing it explicitly as you propose. Best regards, Goetz. From: serviceability-dev [mailto:serviceability-dev-boun...@openjdk.java.net] On Behalf Of Langer, Christoph Sent: Dienstag, 5. Dezember 2017 11:52 To: Chris Plummer <chris.plum...@oracle.com>; serviceability-dev@openjdk.java.net Subject: RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library Hi Chris, thanks for looking at this. The main part of the changes are the findings of a code scan tool (coverity). Here are some detailed comments. I also made a small update to the webrev (concerning eventHelper.c): http://cr.openjdk.java.net/~clanger/webrevs/8192978.1/. > shmemBase.c: seems correct, but could use explanation/example of when > existing code was failing. This is a coverity finding. The correct size specification as per Microsoft’s doc [1] for jlong (__int64) would be I64. VirtualMachineImpl.c: When/why is pos ever null? This is also a coverity case. For some reason, coverity thinks that pos could be NULL here. But I’m with you – I also don’t see a path how that could ever happen. Nevertheless, this change would please converity and I don’t see it being performance critical so I would like to leave it. It’s also not wrong. error_messages.c: Is there any reason not to do this fix for all platforms? Do you have a test case for it? This is a Windows only issue. “vsnprintf“ would write the terminating 0. But in jdk.jdwp.agent/windows/native/libjdwp/util_md.h, vsnprintf is redefined to _vsnprintf. And this, by the Mircrosoft documentation [2], doesn’t necessarily write the terminating character. error_messages.h: ok. looks like just whitespace cleanup yes eventHelper.c: Do you have a testcase? Also, is the "bagAdd(eventBag)" message going to be of use to the user? For this one I don’t have the history. Maybe it was also the coverity tool. But there is no testcase. In my update I have updated 2 other places with the null check where bagAdd is called. I guess in case of such a null error occurring, it could help a little. But probably this would be something to be investigated by the developer rather than the debugging user. inStream.c: inStream_init() now uses a magic number of 11. What does it represent? inStream_endOfInput() looks like it used to be completely broken. How was this not noticed? Is this related to the change inStream_init(), which possible was masking this error? Is there a test case for it? I found this fix in our code base. instream_endOfInput() is obviously never used. At least I don’t find any references in the source code scan. Maybe I shall completely remove it? As for the “magic number of 11”: This is the offset to the payload of a jdwpCmdPacket, see jdk.jdwp.agent/share/native/include/ jdwpTransport.h. (sizeof(jint) /* len*/ + sizeof(jint) /* id */ + sizeof(jbyte) /* flags*/ + sizeof(jbyte) /* cmdSet */ + sizeof(jbyte) /*cmd */ Maybe I should write it down explicitly? With that offset subtracted from the length field, we initialize stream->left with the correct value which makes the checks in readBytes (line 73) more correct. But there is also no test case invoker.c: ok: looks like just whitespace cleanup Yes. log_messages.c: is there any reason not to do this fix for all platforms? Do you have a test case for it? Same goes as for error_messages.c. I’m still hoping to get this change done under the hood of JDK-8192978. I will update the bug with some more details and also add an appropriate noreg label. Best regards, Christoph [1] https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx [2] https://msdn.microsoft.com/en-us/library/1kt27hek.aspx From: Chris Plummer [mailto:chris.plum...@oracle.com] Sent: Montag, 4. Dezember 2017 22:32 To: Langer, Christoph <christoph.lan...@sap.com<mailto:christoph.lan...@sap.com>>; serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net> Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library Hi Christoph, Some of your changes could use explanations. Can you please elaborate on the problems you are fixing in the CR? Test cases, or at least explaining how to reproduce the issues you are fixing would also be useful. shmemBase.c: seems correct, but could use explanation/example of when existing code was failing. VirtualMachineImpl.c: When/why is pos ever null? error_messages.c: Is there any reason not to do this fix for all platforms? Do you have a test case for it? error_messages.h: ok. looks like just whitespace cleanup eventHelper.c: Do you have a testcase? Also, is the "bagAdd(eventBag)" message going to be of use to the user? inStream.c: inStream_init() now uses a magic number of 11. What does it represent? inStream_endOfInput() looks like it used to be completely broken. How was this not noticed? Is this related to the change inStream_init(), which possible was masking this error? Is there a test case for it? invoker.c: ok: looks like just whitespace cleanup log_messages.c: is there any reason not to do this fix for all platforms? Do you have a test case for it? I think you will need to file separate CRs for each issue. You can lump the whitespace cleanup into one. The rest probably should each be seaprate CRs, but I could see possibly doing just one CR for them if the CR documented each problem being fixed. thanks, Chris On 12/4/17 7:44 AM, Langer, Christoph wrote: Hi, I’d like to contribute a few fixes that have been done in our JDK builds that add a few additional checks and cleanups. Can I please get a review. Bug: https://bugs.openjdk.java.net/browse/JDK-8192978 WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.0/<http://cr.openjdk.java.net/%7Eclanger/webrevs/8192978.0/> Thanks, Christoph
