Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

[Chris Plummer]

Hi Christoph, On 12/5/17 2:52 AM, Langer, Christoph wrote: 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. Yes. I was just wondering if using the incorrect format ever produced a test failure.  Also, isn't %ld incorrect for 32-bit platforms? But then I doubt this code has ever been compiled for a 32-bit unix target. Also, it looks like PRId64 should work for all platforms. This is what we use in hotspot to print a 64-bit value: #define INT64_FORMAT           "%" PRId64 So maybe this fix can be simplified. 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. I'm actually more concerned about making the code harder to read and misleading the reader into thinking it could actually be NULL. I'm not familiar with coverity, but I thought tools like this either provided a specific call path for these types of errors (in which case the problem might actually be up the call chain), or allowed you to configure the tool not to complain about specific cases that you know can't happen. 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. Any idea why we do this redefine of vsnprintf? In any case, it might be best to just unconditionally terminate rather than have WIN32 explicit code. 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. I think you should hold off on this one until you have a better handle on why this was needed, and possible also include a test case. 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? Can you use offsetof? 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 Ok. 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. Ok.     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. I think the coverity fixes can be one bug. The format changes one bug (I assume coverity did not find them). inStream.c should have its own bug. eventHelper.c should have its own bug and more investigation as to why it is needed. I get nervous when the reason for a change has been forgotten. thanks, Chris   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>; 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/   Thanks, Christoph