Review request for 8058313: Mismatch of method descriptor and MethodParameters.parameters_count should cause MalformedParameterException

[Eric McCorkle]

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/
>