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

[Ioi Lam]

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.

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