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

Thu Apr 18 06:19:53 PDT 2013

Hi,

I plan to withdraw this change and claim that the bug's test is illegal.

The test calls the Unsafe API to load a corrupted class and then 
complains that the JVM crashes.  But, as many reviewers have pointed 
out, it is ok for the hotspot Unsafe API to crash if passed invalid 
data.  It is the responsibility of the user to pass valid data to the 
Unsafe API's.  Tests that call Unsafe.defineAnonymousClass() with bogus 
data are no more valid than tests that call Unsafe.putAddress(0,0).

Also, I now think it is incorrect to verify an anonymous class if its 
host class does not require verification.  Anonymous classes inherit 
their host class's class loader, protection domain, etc. So, an 
anonymous class should be as trusted as its host class.   To plagiarize 
John Rose, "An anonymous class is intended to enjoy exactly the same 
privileges and limitations as its host class enjoys".

Let me know if you disagree with this decision.  I hope I didn't waste 
too much of people's time.  Thanks for all the comments.

Harold

On 4/14/2013 8:22 AM, Remi Forax wrote:
> 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
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>