RFR 8166358: Re-enable String verification in java_lang_String::create_from_str()

[David Holmes]

LGTM.

Thanks,
David

On 26/05/2020 10:29 pm, coleen.phillimore at oracle.com wrote:
> 
> Here's a new webrev with the name change requested by Harold.
> 
> http://cr.openjdk.java.net/~coleenp/2020/8166358.02/webrev/index.html
> 
> I changed the confusing comment to:
> 
> + // Class resolution will get the class name from the .class stream if 
> the name is null.
> 
> 
> I did some archeology and I think it was talking about comparing the 
> class name given to the one in the .class stream.  It was an old comment.
> 
> thanks,
> Coleen
> 
> 
> 
> On 5/24/20 10:41 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 22/05/2020 10:12 pm, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 5/22/20 7:34 AM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> David, thanks for looking at this.
>>>>
>>>> On 5/22/20 3:26 AM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> Overall this looks good. A few minor comments below.
>>>>>
>>>>> On 22/05/2020 6:34 am, coleen.phillimore at oracle.com wrote:
>>>>>> Summary: Check for invalid strings in class names in debug mode, 
>>>>>> and only verify valid strings in create_for_str().
>>>>>>
>>>>>> See bug for more information on why this commented out code needs 
>>>>>> to be conditional but not commented out. Tested with mach5 
>>>>>> tier1-6, and new test for new -Xcheck:jni check.
>>>>>>
>>>>>> open webrev at 
>>>>>> http://cr.openjdk.java.net/~coleenp/2020/8166358.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8166358
>>>>>
>>>>> src/hotspot/share/classfile/javaClasses.cpp
>>>>>
>>>>> I'm a bit confused about adding the validity check at the end of 
>>>>> java_lang_String::create_from_str. Don't we need to ensure it is 
>>>>> valid before doing those other Utf8 operations on it?
>>>>
>>>> If I return NULL when the string isn't a valid UTF8 string, 
>>>> Cassandra gets a NPE.  I do think it should be fixed, I just don't 
>>>> know how to fix it.
>>>
>>> Oh, I don't think I answered this.  The conversion functions seem to 
>>> do their best and insert utf8 control characters in (see the bug 
>>> comments for an example).  Changing what we do for this case might 
>>> break a lot of programs, so I didn't change it.
>>
>> Okay. Still seems a little odd. :)
>>
>>>
>>>>>
>>>>>     // This check is too strict because the input string is not 
>>>>> necessarily valid UTF8.
>>>>>     // For example, it may be created with arbitrary content via 
>>>>> jni_NewStringUTF.
>>>>> !   if (UTF8::is_legal_utf8((const unsigned char*)utf8_str, 
>>>>> (int)strlen(utf8_str), false)) {
>>>>>
>>>>> The comment needs adjusting now that you are guarding the check 
>>>>> with is_legal_utf8().
>>>>
>>>> I read that comment as explaining why we guard the check. Maybe 
>>>> s/because/when/ above? and remove "not necessarily"?
>>
>> // This check is too strict when the input string is not valid UTF8.
>>
>> Works for me.
>>
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/classfile/systemDictionary.cpp
>>>>>
>>>>> +      return NULL;
>>>>>
>>>>> Indentation appears off by one.
>>>> fixed.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/prims/jvm.cpp
>>>>>
>>>>> I think this comment block:
>>>>>
>>>>> // Since exceptions can be thrown, class initialization can take place
>>>>> // if name is NULL no check for class name in .class stream has to 
>>>>> be made.
>>>>>
>>>>> should be read as:
>>>>>
>>>>> // Since exceptions can be thrown, class initialization can take 
>>>>> place.
>>>>> // If name is NULL no check for class name in .class stream has to 
>>>>> be made.
>>>>>
>>>>> so your changes to the comment:
>>>>>
>>>>> // Since exceptions can be thrown, class initialization can take place
>>>>> // if name is NULL. The class name is checked in the .class stream.
>>>>
>>>> I didn't understand the preexisting comment block at all then.  If 
>>>> the name passed in is NULL, then we must check the class name in the 
>>>> bytecode stream.  How can we not?
>>>>
>>>> By "check" I assume that means "check that it's a valid class name".
>>>>
>>>> Might the comment mean that that if the class_name passed in is NULL 
>>>> we don't check it against the one in the .class stream that it's the 
>>>> same?  How is that at all interesting here in this context?  What is 
>>>> this comment trying to say here?  I assume that it's okay for the 
>>>> class name to be null in these contexts because we get the class 
>>>> name from the .class stream.
>>
>> I'm a bit unclear here as well. I think the comment is saying that 
>> when the class_name passed in is NULL that we don't have to check that 
>> it matches the one in the class stream. But I also don't see the 
>> relevance of such a comment in this context. Perhaps just delete:
>>
>>  320   // Since exceptions can be thrown, class initialization can 
>> take place
>>  321   // if name is NULL no check for class name in .class stream has 
>> to be made.
>>
>> Thanks,
>> David
>> -----
>>
>>>>
>>>> Coleen
>>>>
>>>>>
>>>>> don't appear appropriate.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>
>>>
>