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 20:03:24 UTC 2014
Ah, ok, thanks.
In that case, I need a capital-R reviewer to look at this, please?
Thanks,
Eric
On 11/07/14 14:49, Jiangli Zhou wrote:
> Eric,
>
> I'm not a "R"eviewer yet. You need at least two reviewers, within them
> at least one should be "R"eviewer.
>
> Thanks,
> Jiangli
>
> On 11/07/2014 11:40 AM, Eric McCorkle wrote:
>> 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