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

[Coleen Phillimore]

On 9/19/16 2:47 PM, Ioi Lam wrote:
>
>
> On 9/19/16 2:21 AM, Tobias Hartmann wrote:
>> Hi David,
>>
>> On 19.09.2016 10:30, David Holmes wrote:
>>> On 19/09/2016 6:09 PM, Tobias Hartmann wrote:
>>>> Coleen, David, thanks for the reviews and sorry for the delay!
>>>>
>>>> I agree that we should validate the Strings we get through JNI, but 
>>>> the code in create_from_str() and create_from_symbol() was 
>>>> originally added to check the result of String compression with the 
>>>> Compact Strings feature and not to check for valid UTF-8.
>>>>
>>>> I think UTF-8 validity should be checked earlier (before or during 
>>>> Symbol creation) and this affects other JNI methods that take 
>>>> C-Strings as well (jni_NewString, jni_NewStringUTF, 
>>>> jni_DefineClass, ...). This is a general problem with JNI not 
>>>> validating input Strings.
>>>>
>>>> Are you fine with pushing the proposed fix (webrev.00) and open a 
>>>> follow up bug to fix JNI (and potentially re-enable the Compact 
>>>> Strings asserts because their existence is still justified)?
>>> As I wrote in the bug report:
>>>
>>> "I don't think I agree with the removed checks in either case. JNI 
>>> doesn't do argument checking and so doesn't detect the invalid UTF-8 
>>> string. But the VM expects to work with valid strings. It seems 
>>> quite reasonable to me that the VM should validate the UTF-8 and 
>>> report some kind of failure to make these invalid inputs visible. 
>>> Otherwise the bad JNI codes goes undetected. Maybe an assertion is 
>>> too strong, perhaps we should throw InternalError?"
>> Yes, I've seen that and, as I said, I agree that we should validate 
>> the Strings.
>>
>>> If the VM does not detect this and respond to it exactly what 
>>> happens with the bad UTF-8 string?
>> In the FindClass case, we just return NULL. I don't know if there are 
>> potential problems with invalid UTF-8 strings at other places in the 
>> code.
>>
>>> JNI doesn't do input validation - it is a "feature" of JNI so not to 
>>> penalize correct code.
>> Right, but this check only affects debug builds.
>>
>>> 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 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.

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.

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