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

[Andrey Petushkov]

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

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