Review request (S): 6539281 -Xcheck:jni should validate char* argument to ReleaseStringUTFChars

[Dmitry Samersoff]

Staffan,

My main concern that clients often claim that check_jni is too heavy to 
use it with more-or less loaded application, but we are adding extra 
allocation/copying to one of the most often used JNI call.

Moreover we already copy string two times inside GetStringUTFChars - so 
your copying is the third one.

IMHO we should avoid extra memory manipulations when possible ever at 
the cost of some code duplication.

With non utf string it's easy - just copy part of code from 
GetStringChars replacing copy to compare (see attachment), for UTF it 
also possible but require some experiments before sending to alias ...

-Dmitry







On 2010-12-02 22:11, Staffan Larsen wrote:
> Dmitry,
>
> 1) Yes, I agree. But for ReleaseStringUTFChars a similar check would require a UTF conversion which I'd like to avoid. And I prefer to have similar code in both the UTF and non-UTF case. It has been suggested that the similiar code should be abstracted into some helper method, but I can't figure a way that doesn't make the code more complex and harder to read.
>
> 2) Any ideas?
>
> 3) Yes, "chars" should definitely be NULL-checked.
>
> Thanks,
> /Staffan
>
> On 2 dec 2010, at 18.12, Dmitry Samersoff wrote:
>
>> Staffan,
>>
>> 1. Logically string argument of GetStringChars and ReleaseStringChars have to be the same.
>>   So:
>>     checked_jni_ReleaseStringChars:
>>        chars_to_check = GetStringChars(env,str,isCopy);
>>        memcmp(chars,chars_to_check, len>  10 ? 10 : len);
>>
>> could be a better approach.
>>
>> BUT:
>>
>> 2.
>> As far as I know GetStringChars do alloc/memcpy inside it
>> Could we avoid extra copying?
>>
>> 3.
>> Code below:
>> jint *tagLocation = ((jint*) chars) - 1;
>>
>> Could lead to cryptic crash e.g. if we pass 0 as a char (common case) to this code we will have a crash on read from 0xFFFFFFFF rather than much more clean crash on zero-access. So either gurantee chars != 0 have to be there or tag should be placed at the end of chars, after terminating zero.
>>
>> -Dmitry
>>
>>
>>
>> On 2010-12-02 17:18, Staffan Larsen wrote:
>>> http://cr.openjdk.java.net/~sla/6539281/webrev.00/
>>>
>>> Validate that ReleaseStringUTFChars/ReleaseStringChars is called with
>>> something allocated by GetStringUTChars/GetStringChars when running with
>>> -Xcheck:jni. This is accomplished by adding a well-known tag in the
>>> memory immediately before the pointer that is returned to the user. This
>>> tag is verified in ReleaseStringUTFChars.
>>>
>>> Thanks,
>>>
>>> /Staffan
>>>
>>
>>
>> --
>> Dmitry Samersoff
>> J2SE Sustaining team, SPB04
>> * Give Rabbit time and he'll always get the answer ...
>


-- 
Dmitry Samersoff
J2SE Sustaining team, SPB04
* Give Rabbit time and he'll always get the answer ...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: is_good_string_proposed.cpp
Type: text/x-c
Size: 732 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20101203/c15cae49/attachment.bin