RFR: 8241294: Examine input checking in ClassLoader::package_from_class_name

[Ioi Lam]

Hi Claes,

I think you should change the RFE title to something like "clean up 
ClassLoader::package_from_class_name API".


===============

Since you are removing checks over strings that can potentially have 
arbitrary contents, we need to be extra careful.

You mentioned that

"testing for and skipping ['s is wrong, since fully qualified array 
class names doesn't start with [."

I think you are assuming that this function is always called with a 
valid class name. I checked casually and that seems to be the case, but 
there are a lot of call sites and I have not validated them exhaustively.

InstanceKlass::is_override
    -> InstanceKlass::is_same_class_package()
        -> ClassLoader::package_from_class_name()

java.lang.Class.forName()
    -> ...
       -> SystemDictionary::load_instance_class()
          -> ClassLoader::package_from_class_name()

.....

Class.forName() can be called by user code with arbitrary strings, but 
it's validated by a call to verifyFixClassname/verifyClassname:

http://hg.openjdk.java.net/jdk/jdk/file/5ac19bd3a1e2/src/java.base/share/native/libjava/Class.c#l129

All other cases seem to be names that comes from class names, or 
references from constant pool entries, which should have been checked 
during class file parsing to contain valid names.

Since you're going down this path, I think we should add a stronger 
assertion than just (name[0] != '['). Instead, you should assert with

     (name[0] != '[' && ClassFileParser::verify_legal_class_name(...))

(you would need to refactor the verify_legal_class_name function).

This will make sure all future users of this function won't try to abuse it.

And then, run the JCK to make sure we are indeed catching all cases.

===============

Also, I think you can get rid of this, as it will never happen:

   // in which case start > end
   if (base >= end) {
     return NULL;
   }

================

Then, you can tighten up the comments in the hpp file about the input 
and output (something like):

input -- must be a valid, binary name of an InstanceKlass (array names 
are not allowed)
output -- package of the class, or NULL iff the class is in the default 
package (i.e., the name contains no "/").

Thanks
- Ioi


On 3/27/20 6:07 PM, Claes Redestad wrote:
>
>
> On 2020-03-27 21:47, Lois Foltan wrote:
>> On 3/27/2020 9:55 AM, Claes Redestad wrote:
>>> Hi,
>>>
>>> in ClassLoader::package_from_class_name, testing if the class name 
>>> input
>>> is NULL is redundant, and testing for and skipping ['s is wrong, since
>>> fully qualified array class names doesn't start with [.
>>>
>>> We can also get rid of the bad_class_name parameter, and move
>>> the complexity of that checking to the only call site that 
>>> distinguishes
>>> between an empty package NULL and a bad class name NULL
>>>
>>> Webrev: http://cr.openjdk.java.net/~redestad/8241294/open.00/
>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8241294
>>>
>>> Testing: tier1-4 (ongoing)
>>>
>>> Thanks!
>>>
>>> /Claes
>>
>> Hi Claes,
>> Looks good.  I'm glad to be rid of that "bad_class_name" parameter. 
>> Thank you for doing this. 
>
> Thanks!
>
>> Minor comment:
>>
>> - classfile/classLoader.cpp
>> line #180 & #188 - plase consider adding an improved comment over 
>> "Can't be" in the asserts.  Maybe
>> line #180 "unexpected null class name"
>> line #188 "class name's first character contains an array opening 
>> bracket"
>
> Fixed.
>
>>
>> Also, I was able to write a .jasm file with a class whose name is 
>> "[FOO" for example.  With your patch, I did try executing the 
>> resulting .class file and am receiving a CNFE, "Could not find or 
>> load main class [FOO" exception.  If you would like the .jasm file I 
>> can mail it to you to experiment more with.
>
> Added a sanity test based on runtime/ClassFile/FormatCheckingTest 
> (thanks for the pointer!):
>
> http://cr.openjdk.java.net/~redestad/8241294/open.01/
>
> The test outputs the same ClassFormatError as the old test case
> (LBadHelloWorld; vs [BadHelloWorld2). Running BadHelloWorld2 fails with
> a CNFE if I run without verification, both with and without my patch,
> so I think we have verified that such classes are rejected before ever
> hitting the package_for_class_name.
>
> Thanks,
>
> /Claes
>
>>
>> Thanks,
>> Lois