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

Fri Nov 7 18:33:47 UTC 2014

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/