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

[Ioi Lam]

On 6/16/16 1:51 PM, David Holmes wrote:
> 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));
>      }
>
This one won't work because utf8msg is dynamically allocated, so 
sizeof(utf8msg) == sizeof(jbyte *).

- Ioi

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