Review request for 8058313: Mismatch of method descriptor and MethodParameters.parameters_count should cause MalformedParameterException
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Nov 7 20:26:59 UTC 2014
Eric,
I reviewed this also. This is a bit confusing storing a 0 for
method_parameters_length=0 in ConstMethod but I think it's fine with the
comments.
I have bad news though, you may need to make serviceability agent
changes. You can tell if you run nsk.sajdi.testlist in ute.
Coleen
On 11/7/14, 3:03 PM, Eric McCorkle wrote:
> 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