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

[Jiangli Zhou]

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/