Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

[David Holmes]

On 23/06/2016 11:02 PM, stanislav lukyanov wrote:
> There were different points in the discussion I didn't have a chance to
> comment in time,
> so I'll just summarize everything here not to break the thread flow with
> answers to older messages.
>
> First of all, I'm perfectly fine with either approach (adding name
> validation or not)
> as long as the behavior is clearly documented.
>
> I think it should be specified that unnamed module will be returned even
> if there cannot ever be a package
> with that name. It is not a complicated case to specify, but it brings
> much more clarity.
> Formally, now the function is specified to work with "package names" and
> the behavior on a string that is clearly not a "package name" is
> unspecified.

I think it is completely specified. Something that can not be a valid 
package name can obviously not have a module associated with it and so 
the unnamed-module is always returned.

> The empty string case may or may not be specified. It may be considered
> another corner case, different from both
> "legal" and "illegal" package names, but I personally think that the
> behavior is clear anyway.
>
> I don't think the argument that JVMTI doesn't validate names is correct.
> GetLocalVariableTable doesn't look like a good example here, since
> it just reads data that's already in the VM anyway,
> but in case of GetModuleByPackageName it is about validation of input
> parameters.
> Other APIs that take identifiers like, for example, JNI FindClass or
> GetMethodID
> don't specify name checks explicitly, but throw
> ClassNotFoundError/NoSuchMethodError illegal names.
> It looks like GetModuleByPackageName is the first JNI/JVMTI function to
> succeed when an ill-formed identifier is passed,
> so it deserves to be documented.

I disagree with all of that. GetLocalVariableTable, ClassFileLoadHook, 
DynamicCodeGenerated, GetThreadInfo, to name a few, all take "names" 
encoded as UTF-8 modified strings. None of them validate that the "name" 
is legal for the entity being named - JVM TI simply does not do that 
kind of argument validation.

The JNI functions also do not do argument validation. The JNI spec is 
clear "The JNI does not check for programming errors such as passing in 
NULL pointers or illegal argument types". It is up to the programmer to 
ensure they pass valid arguments. FindClass is specified to simply throw:

NoClassDefFoundError: if no definition for a requested class or 
interface can be found.

It is the internal VM code, that has to deal with bytecode from 
arbitrary sources, that performs the more detailed checking of the name.

GetModuleByPackageName is slightly unusual in that it really never 
fails. As I discussed in the CCC review it could have made a distinction 
between packages that would be loaded by the loader and packages that 
would not, and throw an exception (which in turn may have been able to 
discern that the name was invalid). But that is not the case - if you 
don't pass the name of a package that is known to the loader then you 
get back the unnamed-module. It doesn't matter whether the package name 
is legal-but-unknown, or illegal - it is just unknown.

David
------

> On CCC update: AFAIU CCC needs to have final version of the proposed
> specification, so yes,
> it needs to be updated to with the test that will be actually pushed to
> the workspace.
>
> Thanks,
> Stas
>
> On 23.06.2016 14:40, David Holmes wrote:
>>
>>
>> On 23/06/2016 9:33 PM, serguei.spitsyn at oracle.com wrote:
>>> On 6/23/16 04:27, David Holmes wrote:
>>>> On 23/06/2016 9:08 PM, serguei.spitsyn at oracle.com wrote:
>>>>> On 6/23/16 03:51, David Holmes wrote:
>>>>>> On 23/06/2016 6:04 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>> On 6/23/16 00:51, Alan Bateman wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 23/06/2016 00:20, serguei.spitsyn at oracle.com wrote:
>>>>>>>>> :
>>>>>>>>>
>>>>>>>>> I agree with it.
>>>>>>>>> Thank you for pointing to this JVMTI example.
>>>>>>>>> I did not find in the JNI where the names are checked to be legal.
>>>>>>>>> We are going to open a can of worms with this kind of check as
>>>>>>>>> there
>>>>>>>>> can be many corner cases to cover.
>>>>>>>> The primary use-case for this function is from within the CLFH
>>>>>>>> callback so have it return the unnamed module when calling with
>>>>>>>> garage
>>>>>>>> is probably okay, it just needs to be specified.
>>>>>>>>
>>>>>>>> Will you update the webrev to reflect where we've got to this in
>>>>>>>> this
>>>>>>>> discussion?
>>>>>>>
>>>>>>> I need to undo my fix for additional check.
>>>>>>> My up-to-date understanding is that we have to explicitly specify
>>>>>>> two
>>>>>>> points:
>>>>>>>   - the function returns the unnamed module if the package does not
>>>>>>> exist
>>>>>>>   - the function does not check the package names for validness and
>>>>>>> always returns the unnamed module for illegal package names
>>>>>>>
>>>>>>> It is better to specify these points explicitly to avoid possible
>>>>>>> confusion.
>>>>>>
>>>>>> I don't agree - nothing else refers to invalid "names" in JVM TI. I
>>>>>> don't see any need to call this out here. If the name supplied is not
>>>>>> a legal package name then it will not match - end of story.
>>>>>
>>>>> David,
>>>>>
>>>>> It was the original approach that is in the CCC.
>>>>> You were the one who got confused here, and it convinced me that this
>>>>> needs to be more clear.
>>>>> All these changes are because you started the discussion. :)
>>>>> It was useful anyway.
>>>>
>>>> I never said anything about illegal package names.
>>>
>>> True.
>>> This is my (probably, wrong) attempt to make it more clear.
>>>
>>>>
>>>>> Are you against both clarifications or just the last one?
>>>>> I'm Ok to return it back as it is in the CCC.
>>>>> It will save time on the CCC approval.
>>>>>
>>>>> This is the last addition to the spec:
>>>>>
>>>>> + The unnamed module is returned if the specified package does not
>>>>> exist.
>>>>
>>>> The notion of "package does not exist" is ill-defined. This case is
>>>> already covered by the primary specification.
>>>>
>>>>> + The function does not check if the specified package name is
>>>>> illegal.
>>>>
>>>> This does not need to be stated as it is not stated anywhere else for
>>>> anything else that may have legal and illegal forms.
>>>
>>> Good.
>>> This is a relevant fragment from current version of CCC:
>>>
>>> +      <description>
>>> +        Return the <code>java.lang.reflect.Module</code> object for a
>>> module
>>> +        defined to a class loader that contains a given package.
>>> +        The module is returned via <code>module_ptr</code>.
>>> +        <p/>
>>> +        If a named module is defined to the class loader and it
>>> +        contains the package then that named module is returned,
>>> +        otherwise the unnamed module of the class loader is returned.
>>> +        If the package name is the empty string then this function
>>> +        always returns the unnamed module for the class loader.
>>> +        <p/>
>>
>> As Stanislav said explicitly mentioning the empty string is not really
>> necessary - but I don't see it as harmful.
>>
>>> Does it looks Ok?
>>
>> Yes - as good now as it was in the CCC discussion :)
>>
>> Cheers,
>> David
>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Cheers,
>>>>>> David
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> -Alan
>>>>>>>
>>>>>
>>>
>