[9] RFR(S): Crash with assert: symbol conversion failure in java_lang_String::create_from_symbol()

[Tobias Hartmann]

Ioi, Coleen,

thanks again for the review!

On 19.09.2016 21:36, Coleen Phillimore wrote:
>>>> If the test that triggered this is sending in bad UTF-8 because it expects JNI to do validation then it is an invalid test. I think the asserts serve their purpose exactly - to show where invalid inputs came from. If this was bad user code then it would be up to them to fix it. If it is a bad test then the test should be fixed.
>>> Sure, the test is broken and will be fixed. So you're suggesting to leave the check in create_from_symbol() as it is and re-enable the check in create_from_str()?
>> I also think this is the right approach. Fix (or disable) the test, but leave the C code the way it is.
>>
>> java_lang_String::create_from_str(jchar*, ...) could (potentially??) be called by non JNI code, in which cases we want the assert.
> 
> No, I think you need to leave the commented out code in create_from_str().  This could break testing and it's too late in jdk9 release for non-jigsaw to destabilize the testing.

I agree, re-enabling the create_from_str() assert will cause other problems (see my latest comment in the bug). That's why I proposed to file a new bug for this.

> I also think the similar change needs to be added to create_from_symbol(), as in the proposed fix.   And file a bug to fix the test and another to evaluate whether we should validate strings in JNI and remove the commented out code.

The test is quarantined and a JCK testbug was filed (see JIRA).

> I think the proposed fix is what we should do for now.   This assert is a regression, and our non-checking of JNI strings and symbols is not.

Yes but as David mentioned in another email, this assert is only triggered by a broken test long after the code was pushed. I think it's safe to leave it in and open a new bug to investigate if we can re-enable the create_from_str() assert.

Thanks,
Tobias

> 
> Thanks,
> Coleen
> 
> 
>>
>> Thanks
>> - Ioi
>>
>>> I'm fine with that but as I remember, re-enabling the check in create_from_str() causes other test failures.
>>>
>>> Best regards,
>>> Tobias
>>>
>>>> Sorry.
>>>>
>>>> David
>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>
>>>>> On 12.09.2016 02:47, David Holmes wrote:
>>>>>> On 10/09/2016 6:55 AM, Coleen Phillimore wrote:
>>>>>>> This change is fine because it matches the commented out assert in
>>>>>>> create_from_str().   We should probably figure out what it would take to
>>>>>>> check the characters coming in from JNI and decide whether we should do
>>>>>>> this.  If not, it doesn't make sense to have commented out asserts.
>>>>>>> But this is okay for jdk9.
>>>>>> Grumble, grumble ... both are bad. If the VM doesn't validate this bad UTF-8 then where does it go? And how does the generator of the bad UTF-8 get informed? An assert may be too drastic but can we throw an exception (InternalError?) ?
>>>>>>
>>>>>> David
>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>>
>>>>>>> On 9/9/16 8:42 AM, Tobias Hartmann wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> please review the following patch:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8164561
>>>>>>>> http://cr.openjdk.java.net/~thartmann/8164561/webrev.00/
>>>>>>>>
>>>>>>>> The verification code in java_lang_String::create_from_symbol() that
>>>>>>>> was added by Compact Strings fails because the input symbol does not
>>>>>>>> contain valid UTF8. The problem is that a JCK JNI test passes an
>>>>>>>> invalid UTF8 string as class name to the JNI method "FindClass". In
>>>>>>>> fact, the string contains garbage from reading past array boundaries
>>>>>>>> because of a bug in the test [1]. The JNI spec [2] states that 'name'
>>>>>>>> should be "a fully-qualified class name (that is, a package name,
>>>>>>>> delimited by “/”, followed by the class name). If the name begins with
>>>>>>>> “[“ (the array signature character), it returns an array class. The
>>>>>>>> string is encoded in modified UTF-8".
>>>>>>>>
>>>>>>>> I nevertheless think that we should not crash in the case of an
>>>>>>>> invalid UTF8 string and therefore disabled the verification code with
>>>>>>>> a comment. We did the same for java_lang_String::create_from_str() [3].
>>>>>>>>
>>>>>>>> Tested with failing JCK test and JPRT (running).
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Tobias
>>>>>>>>
>>>>>>>> [1] https://bugs.openjdk.java.net/browse/JCK-7307244
>>>>>>>> [2]
>>>>>>>> https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#FindClass
>>>>>>>>
>>>>>>>> [3]
>>>>>>>> http://hg.openjdk.java.net/jdk9/hs/hotspot/file/d060826d0911/src/share/vm/classfile/javaClasses.cpp#l274
>>>>>>>>
>>
>