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 16:55:56 PDT 2013 

On 04/12/2013 01:32 PM, Remi Forax wrote:
> On 04/12/2013 05:57 PM, harold seigel wrote:
>> 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
>
> This feature is used not only by me but by other dynamic language 
> runtime implementers,
> I think that at least General Electrics Magik use it too because it 
> allows to spit bytecodes very fast at runtime
> (I don't know why Nashorn doesn't use it BTW).
>
The reason I've heard about is security. Nashorn is deliberately not put 
on the bootclasspath, so anything that requires bootclasspath privilege 
cannot be used, including anonymous classes. This is so that Nashorn can 
be used safely even in the presence of a security manager.

Thanks,
Kris

> If you enable bytecode verification, the issue is not only  that 
> verifying the bytecode take time
> in the VM, it's also that generating the StackFrames also take times 
> (the general algorithm is not linear)
> in the compiler of the dynamic runtime.
>
> So yes, it can cause a SIGSEGV but not more that Unsafe.putInt,
> so please don't change the actual behaviour.
>
> cheers,
> Rémi
>
>>
>>
>> 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