[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR 8166358: Re-enable String verification in java_lang_String::create_from_str()
From:       coleen.phillimore () oracle ! com
Date:       2020-05-26 13:07:34
Message-ID: b426882b-ca9f-b581-c6a0-ef1ad60f27e8 () oracle ! com
[Download RAW message or body]

Thanks David and Harold!
Coleen

On 5/26/20 9:01 AM, David Holmes wrote:
> LGTM.
>
> Thanks,
> David
>
> On 26/05/2020 10:29 pm, coleen.phillimore@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@oracle.com wrote:
>>>>
>>>>
>>>> On 5/22/20 7:34 AM, coleen.phillimore@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@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
>>>>>
>>>>
>>

[prev in list] [next in list] [prev in thread] [next in thread]