RFR(S) 8154820 - JDWP: -agentlib:jdwp=help assertion error
David Holmes
david.holmes at oracle.com
Thu Jun 16 11:10:34 UTC 2016
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
>
>
More information about the hotspot-runtime-dev
mailing list