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

harold seigel harold.seigel at oracle.com
Fri Apr 12 08:57:49 PDT 2013


The implication of not verifying is that the production vm may crash.  
It crashes with a SIGSEGV (on Linux) when running the test that caused 
the bug to be created.

Coleen suggested looking at the name of the host class to distinguish 
between a user created anonymous class and a lambda one.  Then, just 
verify the user created ones.  I'll look into this.

Thanks, Harold


On 4/12/2013 11:25 AM, Krystal Mo wrote:
> 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