RFR (S): 8218939: vm/mlvm/anonloader/stress/byteMutation crashed on windows

[David Holmes]

Hi Kim,

Thanks for looking at this.

On 15/02/2019 9:51 am, Kim Barrett wrote:
>> On Feb 14, 2019, at 5:58 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> webrev: http://cr.openjdk.java.net/~dholmes/8218939/webrev
>> bug: https://bugs.openjdk.java.net/browse/JDK-8218939
>>
>> The bytes() in a Symbol are UTF8 but do not form a NUL-terminated C-string. In a couple of places in the classfile parser we are treating it as NUL-terminated:
>>
>> - in name validation we were calling strchr to find the semi-colon, but this could run off through memory if there was no semi-colon (as per the testcase). This is replaced with memchr which takes the expected length.
>>
>> - in formatting the exception message we used %s but instead we need %.*s and pass the length
> 
> And this change to formatting works because we’re dealing with “Modified UTF-8”, which never has embedded NULs, right?

I believe that is the case, but if not then %.*s will (from my reading 
of the C99 standard) stop writing when it hits the first NUL anyway. So 
we could potentially print a partial name. is there a potential issue 
here I'm not seeing where %.*s could be worse than %s?

> src/hotspot/share/classfile/classFileParser.cpp
> 5137         char* c = (char*) memchr(signature, ';', length - 1);
> 
> This is dropping the const-ness of signature. (Arguably a bug in the
> memchr API.) I'd prefer that c be declared "const char*".

Done.

Thanks,
David

> Other than that, looks good.  I don't need another webrev for the
> added const qualifier.
> 
>> There's a minor change to a test to print the exception information as that exposes the fact we were doing things incorrectly e.g:
>>
>> java.lang.ClassFormatError: Illegal class name "p1//BadInterface1\u00f1\u00ab\u00ab\u00ab\u00ab\u00ab\u00ab\u00ab\u00ab\u00ab\u00ab\u00ab\u00ab\u00ab\u00ab\u00ab\u00abU" in class file
>>
>> versus (after bug fix)
>>
>> java.lang.ClassFormatError: Illegal class name "p1//BadInterface1" in class file UseBadInterface1
>>
>> Testing:
>> - re-ran failing test on Windows with seeds known to have caused failures
>> - ran ran failing test on Windows 150 times with random seeds
>> - tiers 1-3
>>
>> Thanks,
>> David
>> -----
> 
>