Request for review: JDK-8011260: fatal error: LineNumberTable attribute has wrong length in class file

Krystal Mo krystal.mo at oracle.com
Fri Apr 12 08:25:35 PDT 2013 

On 04/12/2013 04:56 AM, Coleen Phillimore wrote:
> On 4/12/2013 1:06 AM, serguei.spitsyn at oracle.com wrote:
>> On 4/11/13 7:35 PM, David Holmes wrote:
>>> On 12/04/2013 12:00 PM, David Holmes wrote:
>>>> On 12/04/2013 9:47 AM, serguei.spitsyn at oracle.com wrote:
>>>>> On 4/11/13 4:08 PM, David Holmes wrote:
>>>>>> On 12/04/2013 2:31 AM, serguei.spitsyn at oracle.com wrote:
>>>>>>> The fix looks good modulo the bootstrap anonymous classes.
>>>>>>> How many anonymous classes are loaded by bootstrap CL?
>>>>>>> Is it worth to care about?
>>>>>>
>>>>>> Serguei: Is what worth caring about?
>>>>>>
>>>>>> As has been explained to me we will now verify all anonymous classes
>>>>>> (bootstrap or not). Is it performance you are concerned about?
>>>>>
>>>>> Yes, performance.
>>>>
>>>> Okay. As this would seem to impact all lambda's I also have to be
>>>> concerned about the performance impact.
>>>
>>> Correction this only affects lambdas associated with boot-loader 
>>> types, so less of a concern (at least for JDK 8, but JDK 9 we will 
>>> likely have more internal use of lambda).
>>
>> It looks Ok for now (JDK 8).
>> But it can be reconsidered for JDK 9 if the performance impact for 
>> internal lambda expressions is noticeable.
>> Anyway, it is good to know about such a possible impact.
>
> I think it boils down to whether user code can be used to create an 
> anonymous class or not.  If it can, then we have to verify or it seems 
> to be a security risk to me.  If it can't, what does this testcase 
> that caused the crash do?   Is it invalid?   Can it never happen?
>
Hmm. It's actually possible for external users to create anonymous 
classes, either via sun.misc.Unsafe or via 
sun.invoke.anon.AnonymousClassLoader.
Remi Forax may be a user of the latter class; in fact he's one of the 
authors of that class. It's just that currently we're only using 
anonymous classes to implement MethodHandles.

As the package name shows, both classes are under sun.*, which is 
restricted. We're giving user code the super power to do anything to 
memory already when they can use sun.misc.Unsafe.
What kind of implication is there if we don't verify anonymous classes? 
When run under a security manager and suitable security policy, external 
user code shouldn't be able to create anonymous classes; and when they 
can, they should be able to do whatever they want with Unsafe already.

Thanks,
Kris

> Thanks,
> Coleen
>>
>> Thanks,
>> Serguei
>>
>>>
>>> David
>>>
>>>> David
>>>> -----
>>>>
>>>>> I also wanted to understand what classes are potentially impacted.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>>>
>>>>>> Harold: based on current understanding this looks good to me.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>> On 4/11/13 5:51 AM, harold seigel wrote:
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>> >> /But if I understand the fix correctly we are now verifying all
>>>>>>>> anonymous classes regardless of which loader they are 
>>>>>>>> associated - is
>>>>>>>> that right? /
>>>>>>>>
>>>>>>>> Yes, that is right.  We will now verify all anonymous classes
>>>>>>>> regardless of loader.
>>>>>>>>
>>>>>>>> Thanks, Harold
>>>>>>>>
>>>>>>>> On 4/10/2013 10:42 PM, David Holmes wrote:
>>>>>>>>> On 11/04/2013 12:27 AM, harold seigel wrote:
>>>>>>>>>> Hi David,
>>>>>>>>>>
>>>>>>>>>> Thanks for looking at this.  To clarify, by "anonymous 
>>>>>>>>>> classes" I
>>>>>>>>>> mean
>>>>>>>>>> only classes loaded by the anonymous class loader, not anonymous
>>>>>>>>>> inner
>>>>>>>>>> classes.
>>>>>>>>>
>>>>>>>>> Of course :)
>>>>>>>>>
>>>>>>>>>> Please see in-lined responses.
>>>>>>>>>>
>>>>>>>>>> Thanks, Harold
>>>>>>>>>>
>>>>>>>>>> On 4/9/2013 8:03 PM, David Holmes wrote:
>>>>>>>>>>> Hi Harold,
>>>>>>>>>>>
>>>>>>>>>>> On 9/04/2013 11:55 PM, harold seigel wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> Please review the following bug fix:
>>>>>>>>>>>>
>>>>>>>>>>>> Summary:   The assertion was prevented by changing the code to
>>>>>>>>>>>> verify
>>>>>>>>>>>> classes loaded by the anonymous class loader, unless 
>>>>>>>>>>>> -noverify was
>>>>>>>>>>>
>>>>>>>>>>> So if I understand this right:
>>>>>>>>>>>
>>>>>>>>>>> - previously: loader == NULL -> BytecodeVerificationLocal
>>>>>>>>>>> - now: loader == NULL && anonymous-loader ->
>>>>>>>>>>> BytecodeVerificationRemote
>>>>>>>>>>>
>>>>>>>>>>> in other words we were skipping verification for anonymous 
>>>>>>>>>>> classes.
>>>>>>>>>> Yes, unless full verification (-verify) was explicitly 
>>>>>>>>>> specified or
>>>>>>>>>> their host classes had their own non-null class loaders.
>>>>>>>>>
>>>>>>>>> I'm still a little confused - sorry. Normally we don't verify
>>>>>>>>> boot-loader classes (unless requested). So I would have 
>>>>>>>>> expected that
>>>>>>>>> anonymous classes associated with the boot-loader would also 
>>>>>>>>> not get
>>>>>>>>> verified. But if I understand the fix correctly we are now 
>>>>>>>>> verifying
>>>>>>>>> all anonymous classes regardless of which loader they are 
>>>>>>>>> associated
>>>>>>>>> - is that right?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I guess I don't fully understand the implications here. 
>>>>>>>>>>> Given we
>>>>>>>>>>> normally skip verification for classes loaded by the 
>>>>>>>>>>> boot-loader,
>>>>>>>>>>> does
>>>>>>>>>>> this mean we now verify anonymous classes "linked" to the
>>>>>>>>>>> boot-loader?
>>>>>>>>>>> Are all anonymous classes considered to be non-boot-loader 
>>>>>>>>>>> classes?
>>>>>>>>>>>
>>>>>>>>>> Yes, we would now verify anonymous classes "linked" to the
>>>>>>>>>> boot-loader.
>>>>>>>>>>
>>>>>>>>>> The answer to 'should anonymous calls considered to be
>>>>>>>>>> non-boot-loader
>>>>>>>>>> classes" might be yes and no, depending on the context. And this
>>>>>>>>>> change
>>>>>>>>>> proposes that the answer be 'no' when it comes to whether or 
>>>>>>>>>> not to
>>>>>>>>>> skip
>>>>>>>>>> verification.
>>>>>>>>>>
>>>>>>>>>>> Makes me wonder if there are other places where we check a 
>>>>>>>>>>> loader
>>>>>>>>>>> for
>>>>>>>>>>> NULL but also need to check it is not related to anonymous
>>>>>>>>>>> classes ?
>>>>>>>>>>>
>>>>>>>>>> That's a good point.  Should I create a new bug to look into 
>>>>>>>>>> this?
>>>>>>>>>>>> specified.  Also, the relevant assertion messages were 
>>>>>>>>>>>> improved to
>>>>>>>>>>>> include additional information, including class name.
>>>>>>>>>>>
>>>>>>>>>>> Thanks for that. I hadn't realized the classfile_parse_error
>>>>>>>>>>> routines
>>>>>>>>>>> created and threw exceptions.
>>>>>>>>>>>
>>>>>>>>>>> David
>>>>>>>>>>> -----
>>>>>>>>>>>
>>>>>>>>>>>> Open webrev at 
>>>>>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8011260/
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8011260/>
>>>>>>>>>>>>
>>>>>>>>>>>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=8011260
>>>>>>>>>>>>
>>>>>>>>>>>> The change was tested by hand to ensure that the assertion no
>>>>>>>>>>>> longer
>>>>>>>>>>>> occurs when the test is run.  Additional testing was done 
>>>>>>>>>>>> using
>>>>>>>>>>>> JCK Lang
>>>>>>>>>>>> and VM, JTREG tests, ute vm.quick.testlist and 
>>>>>>>>>>>> vm.mlvm.testlist
>>>>>>>>>>>> tests,
>>>>>>>>>>>> and JPRT tests.
>>>>>>>>>>>>
>>>>>>>>>>>> Thank you!
>>>>>>>>>>>> Harold
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>
>

More information about the hotspot-runtime-dev mailing list