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

[coleen.phillimore at oracle.com]

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