follow-up to 8241294: Examine input checking in ClassLoader::package_from_class_name

[David Holmes]

Hi Andrey,

On 9/06/2020 6:28 pm, Andrey Petushkov wrote:
> Hi,
> 
> AFAICT, it's in fact not the JDK-8153858, it has only moved this code around.
> The '[' and 'L' checks appeared in the initial jigsaw commit by Alan,
> just in a different file (instanceKlass.cpp, (1))

Yep I missed that. Thanks.

Looking at the history there we had:

       // Skip over '['s
       if (*name1 == '[') {
         do {
           name1++;
         } while (*name1 == '[');
         if (*name1 != 'L') {
           // Something is terribly wrong.  Shouldn't be here.
           return false;
         }
       }

(and similarly for name2) which makes a lot more sense. Then the module 
system introduced:

       // Skip over '['s
       if (*base_name == '[') {
         do {
           base_name++;
         } while (*base_name == '[');
         if (*base_name != 'L') {
           // Fully qualified class names should not contain a 'L'.
           // Set length to -1 to indicate that the package name
           // could not be obtained due to an error condition.
           // In this situtation, is_same_class_package returns false.
           length = -1;
           return NULL;
         }
       }

which introduces the strange comment, but checks correctly that [ is 
followed by 'L'. Then 8153858 gave us the code we have today, where the 
comment seems to have been used to change to != to == !!!

> It would probably be too much for me to take the task Claes
> describing, of cleaning up the array class name representations
> globally.
> But something simpler, like trying to duplicate the method into two
> forms (one for "A[]" and one for "[LA;"), I should be able to handle,
> if you'd like.

I'm not at all clear what remains to be done in this area after recent 
signature code changes.

Cheers,
David
-----


> Andrey
> 
> (1) https://hg.openjdk.java.net/jdk9/jdk9/hotspot/file/c558850fac57/src/share/vm/oops/instanceKlass.cpp#l2201
> 
> On Tue, Jun 9, 2020 at 8:03 AM David Holmes <david.holmes at oracle.com> wrote:
>>
>> On 9/06/2020 2:59 am, Claes Redestad wrote:
>>> Hi,
>>>
>>> the method in question has historically been tangled up with use sites
>>> that pass either fully qualified class names ('java/lang/Thread') or
>>> class name signatures (e.g., '[Ljava/lang/Thread').
>>
>> I certainly find the code in question very confusing when looking at it
>> now. It seems to be expecting the FQN form, yet is within logic that
>> already found the array [ and so should always expect that to be
>> followed by 'L'. The code was added in 2016 under JDK-8153858:
>>
>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-June/019767.html
>>
>> I reviewed it at the time, but now I am at a loss to explain it. :(
>>
>> David
>> -----
>>
>>> I have no clarifications to give other than I think the way forward is
>>> to ensure signatures are always parsed into their constituent parts as
>>> they are read from the constant pool and not allowed to roam around in
>>> the VM code acting as fully qualified class names (which they aren't).
>>>
>>> /Claes
>>>
>>> On 2020-06-08 17:20, Andrey Petushkov wrote:
>>>> Dear Claes, All,
>>>>
>>>> one question related to the subject. Even though it's considered
>>>> legitimate for the method to accept '['s at the start of the input
>>>> IMHO the check for subsequent 'L' is wrong and the related comment is
>>>> misleading.
>>>>
>>>> JLS does not prohibit class and package names to start from capital L,
>>>> so it's unclear why arrays of those, and only those (considering these
>>>> appear here in internal form) being banned?
>>>>
>>>> Moreover, given that this function is widely used it's even more
>>>> important that if there is some internal special case which passes
>>>> such forms as argument it's better to be clearly documented, instead
>>>> of saying false things like "Fully qualified class names should not
>>>> contain a 'L'." (1).
>>>>
>>>> However this this code seem to pass quite number of editorials by
>>>> different people and still has the same check. So there might be a
>>>> reason. Given that may I ask for kind clarification of the matter?
>>>>
>>>> Thank you,
>>>> Andrey
>>>>
>>>> [1]
>>>> https://hg.openjdk.java.net/jdk/jdk/file/5efafa45f3b8/src/hotspot/share/classfile/classLoader.cpp#l201
>>>>
>>>>