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