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

Chris Plummer Mon, 11 Dec 2017 01:42:07 -0800

Hi Christoph,

I was just guilty of the same thing on Friday. Dan cleaned it up for me. See JDK-8191229. I believe the process is to change the Fix Version in the backport from 10 to 11, re-open it, and then close as resolved "No an issue" (with a comment as to why this was done). For the main CR, change the Fix Version from 11 to 10, copy the HG Updater info from the backport, close as resolved "Fixed". I think you also need to set "Resolved In Build" to "team".

Chris

On 12/10/17 11:39 PM, Langer, Christoph wrote:

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.plum...@oracle.com>; serviceability-dev@openjdk.java.net
Cc: 'serguei.spit...@oracle.com' <serguei.spit...@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.plum...@oracle.com]
Sent: Samstag, 9. Dezember 2017 01:18
To: Langer, Christoph <christoph.lan...@sap.com>; serviceability-dev@openjdk.java.net
Cc: Lindenmaier, Goetz <goetz.lindenma...@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.plum...@oracle.com]
Sent: Freitag, 8. Dezember 2017 21:52
To: Langer, Christoph <christoph.lan...@sap.com>; serviceability-dev@openjdk.java.net
Cc: Lindenmaier, Goetz <goetz.lindenma...@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/

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

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

Reply via email to