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

[Ioi Lam]

On 6/19/16 9:56 PM, David Holmes wrote:
> 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. I've removed that line and rerunning the tests. If 
everything goes well I'll push.

- Ioi

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