RFR(S): 8192978: Missing checks and small fixes in jdwp library
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Dec 11 13:09:33 UTC 2017
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.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
>> <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/c8d79405/attachment-0001.html>
More information about the serviceability-dev
mailing list