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

[Jiangli Zhou]

Eric,

On 11/06/2014 10:35 AM, Eric McCorkle wrote:
> 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.

Ok, I double checked with the spec and the VM code. The 
'parameters_count' is u1, so it would not overflow using u2. So we are 
fine using u2 here.

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

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.

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/