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

[Daniel D. Daugherty]

Fixed.

Dan


On 12/11/17 2:53 AM, Chris Plummer wrote:

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 <mailto:christoph.lan...@sap.com>>; serviceability-dev@openjdk.java.net <mailto:serviceability-dev@openjdk.java.net> *Cc:* Lindenmaier, Goetz <goetz.lindenma...@sap.com <mailto: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>
    <mailto:christoph.lan...@sap.com>;
    serviceability-dev@openjdk.java.net
    <mailto:serviceability-dev@openjdk.java.net>
    *Cc:* Lindenmaier, Goetz <goetz.lindenma...@sap.com>
    <mailto: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/
        <http://cr.openjdk.java.net/%7Eclanger/webrevs/8192978.2/>

        Bug: https://bugs.openjdk.java.net/browse/JDK-8192978
        <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