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

Mon Dec 11 07:39:38 UTC 2017

Hi guys,

I just pushed this: http://hg.openjdk.java.net/jdk/jdk/rev/8db54e2c453b

However, for some reason somebody set my original bug to Fix version 11 which I kinda ignored and now the system created a backport bug for 10 (https://bugs.openjdk.java.net/browse/JDK-8193305). Probably I should have corrected the fix version before I pushed. Now, shall I set the original bug https://bugs.openjdk.java.net/browse/JDK-8192978 to “Resolved” manually or wait for the system to do it? Any hints on that one?

Thanks
Christoph

From: Langer, Christoph
Sent: Samstag, 9. Dezember 2017 18:08
To: 'Chris Plummer' <chris.plummer at oracle.com>; serviceability-dev at openjdk.java.net
Cc: 'serguei.spitsyn at oracle.com' <serguei.spitsyn at oracle.com>
Subject: RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Hi Chris,

thanks for testing. I added the files with whitespace changes to the bug. I’ll push this on Monday.

Best regards
Christoph

From: Chris Plummer [mailto:chris.plummer at oracle.com]
Sent: Samstag, 9. Dezember 2017 01:18
To: Langer, Christoph <christoph.langer at sap.com<mailto:christoph.langer at sap.com>>; serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net>
Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>>
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

All testing passed.

thanks,

Chris

On 12/8/17 1:07 PM, Langer, Christoph wrote:
Thanks in advance, Chris, for testing.

Please let me know the outcome when you are done.

Best regards
Christoph

From: Chris Plummer [mailto:chris.plummer at oracle.com]
Sent: Freitag, 8. Dezember 2017 21:52
To: Langer, Christoph <christoph.langer at sap.com><mailto:christoph.langer at sap.com>; serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net>
Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com><mailto:goetz.lindenmaier at sap.com>
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Hi Christoph,

On 12/8/17 5:48 AM, Langer, Christoph wrote:
Hi,

this is a new webrev for 8192978. This change now only contains the coverity fixes.

In detail:
src/jdk.jdwp.agent/share/native/libjdwp/VirtualMachineImpl.c, static void writePaths(PacketOutputStream *out, char *string):
    strchr could be called with NULL argument because of assignment pos = psPos; in line 856 (last line of for loop). Proposal: check pos for NULL in head of for loop
If I understand correctly how the code currently works, pos/psPos will be NULL on the last iteration of the loop, but we'll exit anyway since "i == npaths" by that point. So the NULL pos will never be referenced. I guess coverity is cluing in on the following section:

 846         psPos = strchr(pos, ps[0]);
 847         if ( psPos == NULL ) {
 848             plen = (int)strlen(pos);
 849         } else {
 850             plen = (int)(psPos-pos);
 851             psPos++;
 852         }

We admit in line 847 that psPos can be NULL, but coverity can't read into the loop logic deep enough to recognize we'll also exit when this happens.

I guess your fix is fine, although technically unnecessary.

 src/jdk.jdwp.agent/share/native/libjdwp/error_messages.c, static void vprint_message(FILE *fp, const char *prefix, const char *suffix, const char *format, va_list ap):
    potentially unterminated vsnprintf call. Proposal: terminate
Ok.

src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c, static jboolean synthesizeUnloadEvent(void *signatureVoid, void *envVoid):
    checking eventBag for NULL and then calling JDI_ASSERT only in that case is a bit dubious.
    It leads the coverity code scan tool to think that eventBag might be NULL when eventHelper_recordClassUnload is called
    which would eventually try to dereference a NULL eventbag and hence crash.
    Proposal: remove the NULL check but unconditionally assert.

    This is the real fix for the changes in eventHelper.c in my former webrev.
Looks good. Thanks for looking deeper into this one.

src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c, void log_message_end(const char *format, ...):
    potentially unterminated vsnprintf call. Proposal: terminate
Ok.

Furthermore I have 2 whitespace changes which I’d like to see because they would help me reducing the diff to our codebase and, furthermore, make the code looking nicer… a little ;-)
Please list the files with just whitespace changes in the CR.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.2/<http://cr.openjdk.java.net/%7Eclanger/webrevs/8192978.2/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

I’m doing builds on the main platform and running jtreg tests.
I'm doing a pretty comprehensive round of serviceability testing myself with these changes and also 8193258. I'll let you know if there are any issues.

thanks,

Chris

Thanks in advance & Best regards
Christoph

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