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

[Dmitry Samersoff]

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.