[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(S) 8154820 - JDWP: -agentlib:jdwp=help assertion error
From:       Ioi Lam <ioi.lam () oracle ! com>
Date:       2016-06-16 7:33:22
Message-ID: 57625642.1070109 () oracle ! com
[Download RAW message or body]



On 6/15/16 7:59 PM, David Holmes wrote:
> Hi Ioi,
>
> On 16/06/2016 8:14 AM, Ioi Lam wrote:
>> Oops. Please ignore my last mail. This one uses the correct mailing 
>> lists.
>>
>> - Ioi
>>
>> =============================================
>> On 6/15/16 3:06 PM, Ioi Lam wrote:
>>
>> Please review a small fix:
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8154820
>> Desc: JDWP: -agentlib:jdwp=help assertion error
>> Fix: http://cr.openjdk.java.net/~iklam/jdk9/8154820_jdwp_help_exit.v02/
>
> Changes look good!
>
> I find this code generally seems to confuse lengths and indices. There 
> is another usage in
>
> ./jdk.jdwp.agent/share/native/libjdwp/transport.c
>
> that seems suspect:
>
>         /* Convert this string to UTF8 */
>         len = (int)strlen(msg);
>         maxlen = len+len/2+2; /* Should allow for plenty of room */
>         utf8msg = (jbyte*)jvmtiAllocate(maxlen+1);
>         if (utf8msg != NULL) {
>            (void)utf8FromPlatform(msg, len, utf8msg, maxlen);
>            utf8msg[maxlen] = 0;
>         }
>
> This should be passing in maxlen+1 as the actual buffer length.
>
Hi David,

Thanks for pointing that out. I've added the following change and I'm 
rerunning the tests now.

$ hg diff transport.c
diff -r dfcdb0a45822 src/jdk.jdwp.agent/share/native/libjdwp/transport.c
--- a/src/jdk.jdwp.agent/share/native/libjdwp/transport.c    Mon Jun 13 
09:03:32 2016 -0400
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/transport.c    Thu Jun 16 
00:31:19 2016 -0700
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 1998, 2015, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2016, Oracle and/or its affiliates. All rights 
reserved.
   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
   *
   * This code is free software; you can redistribute it and/or modify it
@@ -67,7 +67,7 @@
          maxlen = len+len/2+2; /* Should allow for plenty of room */
          utf8msg = (jbyte*)jvmtiAllocate(maxlen+1);
          if (utf8msg != NULL) {
-           (void)utf8FromPlatform(msg, len, utf8msg, maxlen);
+           (void)utf8FromPlatform(msg, len, utf8msg, maxlen+1);
             utf8msg[maxlen] = 0;
          }
      }






> Thanks,
> David
>
>>
>> The were two issues:
>>
>> [1] MAXPATHLEN is really small on Windows (~256), so MAX_MESSAGE_LEN
>>     was around 1024, but the jdwp help message is about 1600 bytes.
>>     The fix would expand it to abut 2500 bytes which is more than 
>> enough.
>>
>> [2] The meaning of the "outputMaxLen" parameter to the utf8ToPlatform()
>>     and utf8FromPlatform() functions was ambiguous. The assert
>>
>>        UTF_ASSERT(outputMaxLen > len);
>>
>>     suggest that the trailing zero is included in the buffer size, so
>>     at most outputMaxLenbytes are written.
>>
>>     However, the implementation actually would write outputMaxLen+1 
>> bytes
>>     in the worst case.
>>
>>     Luckily, this does not seem to be a problem as all calls to
>>     utf8ToPlatform/utf8FromPlatform allocate way more space than
>>     needed. The only exception was vprint_message, which chose to pass
>>
>>       outputMaxLen := sizeof(buf) -1
>>
>>     So in the worst case, the VM would exit due to the above UTF_ASSERT,
>> but
>>     we would not silently write one byte past the end of the buffer.
>>
>>     The fix is:
>>     [a] Clarify the meaning of the outputMaxLen parameter
>>     [b] Make sure we don't write past the end of the buffer
>>     [c] Fix vprint_message() to pass in the correct size of the
>>         buffer to include space for the trailing zero.
>>
>> Thanks
>> Ioi

[prev in list] [next in list] [prev in thread] [next in thread]