RFR(S) 8154820 - JDWP: -agentlib:jdwp=help assertion error

[David Holmes]

Hi Ioi,

On 17/06/2016 1:04 AM, Ioi Lam wrote:
> Hi Dmitry & David,
>
> Thanks for the comments.
>
> http://cr.openjdk.java.net/~iklam/jdk9/8154820_jdwp_help_exit.v03/
>
> I have updated all the code that deals with utf8ToPlatform/utf8FromPlatform
> to use "size of the buffer" (same as snprintf) and not "length of the
> string".
>
> The identifier names have been changed to clarify this:
>
>    MAX_MESSAGE_LEN -> MAX_MESSAGE_BUF
>    outputMaxLen    -> outputBufSize
>
> I also changed the TTY_MESSAGE() calls to avoid using very long
> messages, so there's no longer need for MAXPATHLEN*8 on Windows.

Looks much cleaner and clearer now thanks. I have one nit:

src/jdk.jdwp.agent/share/native/libjdwp/transport.c. This:

      if (utf8msg != NULL) {
         (void)utf8FromPlatform(msg, len, utf8msg, maxlen+1);
         utf8msg[maxlen] = 0;
      }

can be:

      if (utf8msg != NULL) {
         (void)utf8FromPlatform(msg, len, utf8msg, (int)sizeof(utf8msg));
      }

we avoid restating the maxlen+1, and utf8FromPlatform always seems to 
NUL-terminate so we don't need to do it externally.

Thanks,
David

> Please review.
>
> Thanks
> - Ioi
>
> On 6/16/16 4:32 AM, Dmitry Samersoff wrote:
>> David,
>>
>>> - MAX_MESSAGE_LEN is the maximum number of actual characters (not
>>> including NUL)
>> I'm OK with either solution. We can ensure that all buffers is
>> MAX_MESSAGE_LEN + 1, e.g add
>>
>> #define MESSAGE_BUF_LEN  MAX_MESSAGE_LEN + 1
>>
>> and use it.
>>
>> But I would like to avoid a mix of two type of guarding - at variable
>> define time and at variable use time.
>>
>> -Dmitry
>>
>> On 2016-06-16 14:10, David Holmes wrote:
>>> On 16/06/2016 6:00 PM, Dmitry Samersoff wrote:
>>>> Ioi,
>>>>
>>>> As soon as you touched this code, few minor nits.
>>>>
>>>> error_messages.c:
>>>>
>>>> 54:
>>>>    MAXPATHLEN*8 has no sence for me. It might be better to just
>>>>    use a constant with an appropriate comment.
>>>>
>>>> 68:
>>>>   We should either add 1 when allocating buffer or substact 1 when
>>>>   setting termination zero.
>>>>
>>>>      But now we are adding 1 at error_messages.c:70 then
>>>>   substracting it at utf_util.c:365
>>>>
>>>>
>>>>   So please, change 68, 70 to use MAX_MESSAGE_LEN
>>>>   And 74 to use MAX_MESSAGE_LEN - 1
>>> The way I see this:
>>>
>>> - MAX_MESSAGE_LEN is the maximum number of actual characters (not
>>> including NUL)
>>> - So the buffer size is MAX_MESSAGE_LENGTH+1 to allow for NUL.
>>> - vsnprintf reads up to MAX_MESSAGE_LENGTH _but_ that includes the
>>> terminating NUL (stored from 0 to MAX_MESSAGE_LEN-1)
>>> - utf8buf[MAX_MESSAGE_LEN] = 0; also terminates with NUL
>>>
>>> so we end up with two NULs at the end of utf8buf! To me the correct fix
>>> here is similar to what Ioi did at L78:
>>>
>>> (void)vsnprintf((char*)utf8buf, (int)sizeof(utf8buf), format, ap);
>>>
>>> and delete L74 as it is already NUL-terminated.
>>>
>>> I didn't go through the rest of your comments.
>>>
>>> David
>>> -----
>>>
>>>
>>>>   Changes at ll. 78 is not necessary.
>>>>
>>>>
>>>> utf_util.c:
>>>>
>>>> 364:
>>>>    It might be better to change the order of asserts -
>>>>
>>>>     UTF_ASSERT(utf8);
>>>>     UTF_ASSERT(len >= 0);
>>>>    ...
>>>>
>>>> 365.
>>>>     it might be better to just use outputMaxLen -1 at ll. 374
>>>>
>>>> -Dmitry
>>>>
>>>> On 2016-06-16 01:06, 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/
>>>>>
>>>>>
>>>>> 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
>>>>
>>
>