RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class

[harold seigel]

Hi Lois,

Thanks for the review.  Please see comments inline.


On 8/3/2016 12:22 PM, Lois Foltan wrote:
> Hi Harold,
>
> Looks good.  Have some comments:
>
> src/share/vm/classfile/classFileParser.cpp:
> - lots of comments refer to the "null package", can you change that to 
> the "unnamed package".  There really isn't a concept of a null package.
done
> - line #5423 & 5439, why do you use _class_name->base instead of 
> _class_name->as_C_string()?
The name of the anonymous class can contain non-ASCII characters so 
as_C_string() may not work properly.
> - line #5439 - I recall that using jbyte should be avoided, can you 
> change to char?
I used jbyte* because that is the type returned by UTF8::strrchr.  I can 
cast the result to char*, if you prefer.
> - line #5439 - why do you use UTF8::strrchr instead of strrchr like 
> line #5410?
I used UTF8::strrchr because it takes a length argument.
> - line #5451 - extra line
> - line #5455 - can you change the call to 
> InstanceKlass::is_same_class_package to use the associated with "this" 
> or the anonymous class instead of assuming that host_klass and 
> _class_name have the same class loader?  One of the checks that 
> InstanceKlass::is_same_class_package does is to make sure the class 
> loaders are the same and kick out if they are not.  So by passing in 
> the same host_klass class_loader you are bypassing that check.
The anonymous class is always loaded by the same class loader as its 
host.  Method Unsafe_DefineAnonymousClass_impl() passes the host class's 
loader to SystemDictionary::parse_stream() which uses the host loader to 
load the anonymous class.  So the extra class loader check is not 
needed.  Also, the anonymous class is not yet loaded at this point, so 
what class loader would be passed to that is_same_class_package() method.
>
> Testing:
> Is it possible to have nested anonymous classes?  If yes, can you add 
> a test case for this where the host_klass being in one package and 
> then at each nested anonymous class there are differing combinations 
> of unnamed package, named package.
Yes, it is possible to have nested anonymous classes.  An anonymous 
class that does string concatenation would be an example.  But I'm not 
sure such a test would have much value for this issue.
>
> Thanks,
> Lois
>
Thanks, Harold
> On 8/3/2016 8:15 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this fix for bug 8058575.  The fix prevents a class 
>> created using Unsafe.defineAnonymousClass() from being in a different 
>> package than its host class.  Being in different packages would 
>> create access problems if the packages were in different modules.
>>
>> With this fix, If the anonymous class is in a different package then 
>> the JVM will throw IllegalArgumentException.  If the anonymous class 
>> is in the unnamed package then the JVM will move the anonymous class 
>> into its host class's package.
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8058575
>>
>> Open webrevs:
>>
>>    http://cr.openjdk.java.net/~hseigel/bug_8058575.hs/
>>
>>    http://cr.openjdk.java.net/~hseigel/bug_8058575.jdk/
>>
>> The fix was tested with the JCK Lang and VM tests, the hotpot, and 
>> java/lang, java/util and other JTreg tests, the NSK quick tests, and 
>> with the RBT runtime nightly tests.
>>
>> Thanks, Harold
>>
>