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

[David Holmes]

On 17/06/2016 2:24 PM, Ioi Lam wrote:
>
>
> 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 *).

Doh! Missed that. But we still don't need:

  utf8msg[maxlen] = 0;

Thanks,
David


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