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

[Chris Plummer]

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