Review request for 8058313: Mismatch of method descriptor and MethodParameters.parameters_count should cause MalformedParameterException
Eric McCorkle
eric.mccorkle at oracle.com
Fri Nov 7 19:40:16 UTC 2014
Thanks,
Are you a capital R? If not, wouldn't that mean I need two?
On 11/07/14 13:33, Jiangli Zhou wrote:
> Hi Eric,
>
> Looks okay. You also need a capital R reviewer for the change.
>
> Thanks,
> Jiangli
>
> On 11/07/2014 05:07 AM, Eric McCorkle wrote:
>> On 11/06/14 18:53, Jiangli Zhou wrote:
>>
>>> Could you please point to the updated webrev? I don't see the update in
>>> http://cr.openjdk.java.net/~emc/8058313/webrev.01/src/share/vm/prims/jvm.cpp.sdiff.html.
>>>
>> I made a mistake uploading it. It's here:
>> http://cr.openjdk.java.net/~emc/8058313/webrev.02/
>>
>>> Thanks,
>>> Jiangli
>>>>> On 11/03/2014 01:35 PM, Eric McCorkle wrote:
>>>>>> Please review this issue so that it can go in along with 8058322.
>>>>>> Thanks.
>>>>>>
>>>>>> On 10/30/14 19:40, Eric McCorkle wrote:
>>>>>>> Thank you for the pointers. I have applied your changes and
>>>>>>> refreshed
>>>>>>> the webrev.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~emc/8058313/
>>>>>>>
>>>>>>> Also, I have posted the test for this and another patch here:
>>>>>>> http://cr.openjdk.java.net/~emc/8062556/
>>>>>>>
>>>>>>> On 10/30/14 13:51, Jiangli Zhou wrote:
>>>>>>>> Hi Eric,
>>>>>>>>
>>>>>>>> On 10/30/2014 07:56 AM, Eric McCorkle wrote:
>>>>>>>>> On 10/29/14 20:36, Jiangli Zhou wrote:
>>>>>>>>>> Hi Eric,
>>>>>>>>>>
>>>>>>>>>> I wonder if we could specialize this particular case and avoid
>>>>>>>>>> changing
>>>>>>>>>> the parsing code. How about setting the _has_method_parameters
>>>>>>>>>> flag in
>>>>>>>>>> the ConstMethod when encounter such MethodParameter, and changing
>>>>>>>>>> JVM_GetMethodParameters() to return non-NULL value for such case
>>>>>>>>>> when
>>>>>>>>>> _has_method_parameters is true but method_parameters_length is 0.
>>>>>>>>>> Would
>>>>>>>>>> that work?
>>>>>>>>> Which parser are you talking about? The inline tables parser, or
>>>>>>>>> the
>>>>>>>>> class file parser. The class file parser has to change,
>>>>>>>>> because it
>>>>>>>>> was
>>>>>>>>> previously ignoring MethodParameters attributes with
>>>>>>>>> parameter_count 0.
>>>>>>>> It's the class parsing changes that I was referring to, mostly
>>>>>>>> relate to
>>>>>>>> the initialization and checking against method_parameters_length.
>>>>>>>> It's a
>>>>>>>> bit awkward to include the 0 case but also skipping it in the
>>>>>>>> loop. For
>>>>>>>> example, the following code in classFileParser.cpp changed ">" to
>>>>>>>> ">="
>>>>>>>> in the if check, but has no real effect and is not need.
>>>>>>>>
>>>>>>>> 2486 // Copy method parameters
>>>>>>>> 2487 if (method_parameters_length >= 0) {
>>>>>>>> 2488 MethodParametersElement* elem =
>>>>>>>> m->constMethod()->method_parameters_start();
>>>>>>>> 2489 for (int i = 0; i < method_parameters_length; i++) {
>>>>>>>> 2490 elem[i].name_cp_index =
>>>>>>>> Bytes::get_Java_u2(method_parameters_data);
>>>>>>>> 2491 method_parameters_data += 2;
>>>>>>>> 2492 elem[i].flags =
>>>>>>>> Bytes::get_Java_u2(method_parameters_data);
>>>>>>>> 2493 method_parameters_data += 2;
>>>>>>>> 2494 }
>>>>>>>> 2495 }
>>>>>>>>
>>>>>>>>
>>>>>>>>> I don't think your proposal will work. The inline tables'
>>>>>>>>> offsets are
>>>>>>>>> all dependent on what inline tables are actually present. If
>>>>>>>>> _has_method_parameters is set, then the inline tables code
>>>>>>>>> expects the
>>>>>>>>> last u2 of the inline tables to be a u2 indicating the number of
>>>>>>>>> method
>>>>>>>>> parameters entries, preceeded by the array of method parameters
>>>>>>>>> data.
>>>>>>>>> If _has_method_parameters is false, then it expects that there
>>>>>>>>> is no
>>>>>>>>> method parameters information at all (including no length
>>>>>>>>> field). If
>>>>>>>>> you were to set _has_method_parameters, but not store any
>>>>>>>>> information in
>>>>>>>>> the inline table, then it would cause errors for all the rest
>>>>>>>>> of the
>>>>>>>>> inline tables.
>>>>>>>> Thank you for reminding me of the complexity of the inlined table
>>>>>>>> calculation in the ConstMethod. My proposal would require tweaks in
>>>>>>>> that
>>>>>>>> area to correctly compute the table sizes. As it's easy to
>>>>>>>> introduce
>>>>>>>> bugs in that area, it's not worth to change the table calculation
>>>>>>>> code
>>>>>>>> for this purpose. I agree my proposal is not a better choice in
>>>>>>>> this
>>>>>>>> case.
>>>>>>>>
>>>>>>>>> What I do for the parameter_count = 0 case is just store
>>>>>>>>> a 0 u2 for zero-length method parameters information, and no data.
>>>>>>>>> All
>>>>>>>>> the existing inline tables code works fine with this case, so
>>>>>>>>> there
>>>>>>>>> aren't any serious changes to the inline tables code (other than
>>>>>>>>> allowing method parameters information to be stored when the
>>>>>>>>> array is
>>>>>>>>> length 0). But you have to make some change to the inline table
>>>>>>>>> code,
>>>>>>>>> otherwise the information won't be stored.
>>>>>>>> Ok. Could you please add comments to the change in
>>>>>>>> constMethod.cpp to
>>>>>>>> explain above?
>>>>>>>>
>>>>>>>> In jvm.cpp, since -1 represents no method parameter now. Maybe
>>>>>>>> checking
>>>>>>>> against explicity and add comments for the 0-length case.
>>>>>>>>
>>>>>>>> JVM_ENTRY(jobjectArray, JVM_GetMethodParameters(JNIEnv *env,
>>>>>>>> jobject
>>>>>>>> method))
>>>>>>>> {
>>>>>>>> ...
>>>>>>>> // No method parameter
>>>>>>>> if (num_params == -1) {
>>>>>>>> return (jobjectArray)NULL;
>>>>>>>> }
>>>>>>>>
>>>>>>>> /* handle the rest here */
>>>>>>>> // make sure all the symbols are properly formatted
>>>>>>>> for (int i = 0; i < num_params; i++) {
>>>>>>>> ...
>>>>>>>> }
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Jiangli
>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Jiangli
>>>>>>>>>>
>>>>>>>>>> On 10/29/2014 03:39 PM, Eric McCorkle wrote:
>>>>>>>>>>> Hello,
>>>>>>>>>>>
>>>>>>>>>>> Please review this fix for parameter reflection which addresses
>>>>>>>>>>> hotspot
>>>>>>>>>>> falsely ignoring zero-length MethodParameter attributes. The
>>>>>>>>>>> JVMS
>>>>>>>>>>> allows a MethodParameters attribute with parameter_count = 0,
>>>>>>>>>>> and
>>>>>>>>>>> the
>>>>>>>>>>> parameter reflection spec states that a
>>>>>>>>>>> MalformedParametersException
>>>>>>>>>>> should be thrown if parameter_count does not match the number of
>>>>>>>>>>> real
>>>>>>>>>>> parameters to a method. Hotspot currently ignores
>>>>>>>>>>> MethodParameters
>>>>>>>>>>> attributes with parameter_count = 0; however, in a case where a
>>>>>>>>>>> (bad)
>>>>>>>>>>> MethodParameters attribute has parameter_count = 0, but the
>>>>>>>>>>> method
>>>>>>>>>>> has a
>>>>>>>>>>> nonzero number of real parameters, hotspot will return null from
>>>>>>>>>>> JVM_GetMethodParameters, the result being that a
>>>>>>>>>>> MalformedParametersException is not thrown (rather, the
>>>>>>>>>>> reflection API
>>>>>>>>>>> acts like there is no MethodParameters attribute).
>>>>>>>>>>>
>>>>>>>>>>> This patch causes hotspot to record the fact that a zero-length
>>>>>>>>>>> MethodParameters attribute does exist, causing the exception
>>>>>>>>>>> to be
>>>>>>>>>>> thrown when it should be.
>>>>>>>>>>>
>>>>>>>>>>> The bug is here:
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8058313
>>>>>>>>>>>
>>>>>>>>>>> The webrev is here:
>>>>>>>>>>> http://cr.openjdk.java.net/~emc/8058313/
>
More information about the hotspot-dev
mailing list