RFR(S): Crash with assert: symbol conversion failure in java_lang_String::create_from_symbol()
david.holmes at oracle.com
Mon Sep 19 08:30:06 UTC 2016
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? "
If the VM does not detect this and respond to it exactly what happens
with the bad UTF-8 string?
JNI doesn't do input validation - it is a "feature" of JNI so not to
penalize correct code. 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.
> 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?) ?
>>> On 9/9/16 8:42 AM, Tobias Hartmann wrote:
>>>> please review the following patch:
>>>> 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 . The JNI spec  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() .
>>>> Tested with failing JCK test and JPRT (running).
>>>>  https://bugs.openjdk.java.net/browse/JCK-7307244
More information about the hotspot-dev