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

[Coleen Phillimore]

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?

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