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

[David Holmes]

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