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

[Remi Forax]

On 04/13/2013 01:55 AM, Krystal Mo wrote:
> 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.

That make sense.
It depends if you deliver your runtime as a jar or as a bundle with the 
VM included.
>
> Thanks,
> Kris

cheers,
Rémi

>
>> 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
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>