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

[Eric McCorkle]

On 11/04/14 20:35, Jiangli Zhou wrote:
> Hi Eric,
> 
> I have a few more comments:
> 
> In ClassFileParser::parse_method(), should 'real_length' be int instead
> of u2?

No.  By that point, we know it's positive, and it's about to be compared
to method_attribute_length, which is a 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).

Appiled, and webrev refreshed.  Please look at it.

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