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

[coleen.phillimore at oracle.com]

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.
>
>     // 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"?
>
> ---
>
> 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.

Coleen

>
> don't appear appropriate.
>
> Thanks,
> David
> -----
>
>> Thanks,
>> Coleen