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

[Langer, Christoph]

Hi Chris,

thanks again for your reply.


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

You are probably right, I’ll address this in my next webrev.

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.

OK, what I’m gonna do now: I’ve reverted the stuff in our codebase to get a new coverity run. If coverity still complains I’ll check again to first of all get an explanation by coverity what should be wrong and then find a more sensible fix. Or maybe we can tell coverity to shut up…

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.

After reading the specs again, I think that windows uses _vsnprintf since this version matches the standard Unix/Linux behavior better which means that the buffer is written up to count and a terminating 0 might not be written. So it’s best to terminate the buffer in any case which will be reflected by my new webrev.

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.

This one is coverity, too. I’m now doing the same approach like I do with VirtualMachineImpl.c. I’m removing the stuff and see if coverity will complain again and then rethink the patch.

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?

Yes, will do in next webrev.

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.

Ok, I will eventually split as suggested.

So, I’ll check the coverity results tomorrow and then come up with new webrevs.

Best regards,
Christoph

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20171206/f8f6b672/attachment-0001.html>