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

Fri Nov 7 19:49:36 UTC 2014

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/