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

[Jiangli Zhou]

Hi Eric,

I have a few more comments:

In ClassFileParser::parse_method(), should 'real_length' be int instead 
of u2?

In JVM_GetMethodParameters(), can you add an assert to make sure the 
num_params is -1 when it's less than 0? Also, it's probably more 
conventional to use (num_params < 0) instead of (0 > num_params).

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/