RFR: JDK-8230776: Javac throws AssertionError in jvm.Gen.visitExec

[Jan Lahoda]

Looks good to me.

Jan

On 28. 08. 20 18:20, Vicente Romero wrote:
> Hi Jan,
> 
> I agree with your comments, I have uploaded another iteration at [1]. 
> The only changes are in PoolReader,
> 
> Thanks,
> Vicente
> 
> [1] http://cr.openjdk.java.net/~vromero/8230776/webrev.02/
> 
> On 8/28/20 9:57 AM, Jan Lahoda wrote:
>> Hi Vicente,
>>
>> I think it looks great overall.
>>
>> Basically my only comment is for PoolReader. Given the performance of 
>> this code is potentially important, would it make sense to make the 
>> validity checks as fast as possible? E.g. having something like:
>> readIfNeeded(int index, BitSet expectedTags) {
>>
>> ...
>> if (!expectedTags.get(currentTag)) {
>>     throw reader.badClassFile("unexpected.const.pool.tag.at", 
>> tag(index), offset(index));
>> }
>>
>> And having static final pre-created BitSets used to pass into this 
>> method?
>>
>> Thanks,
>>     Jan
>>
>> On 28. 08. 20 0:01, Vicente Romero wrote:
>>> Hi Jan,
>>>
>>> Please review another iteration of the patch at [1], this one is a 
>>> bit more complex as it addresses several very related bugs together. 
>>> These bugs are [2-6]. The common factor is that javac interrupts 
>>> compilation with an exception or assertion error while compiling a 
>>> source file that extends a fuzzed class file.
>>>
>>> Thanks,
>>> Vicente
>>>
>>> [1] http://cr.openjdk.java.net/~vromero/8230776/webrev.01/
>>> [2] https://bugs.openjdk.java.net/browse/JDK-8231311
>>> [3] https://bugs.openjdk.java.net/browse/JDK-8230964
>>> [4] https://bugs.openjdk.java.net/browse/JDK-8230963
>>> [5] https://bugs.openjdk.java.net/browse/JDK-8230919
>>> [6] https://bugs.openjdk.java.net/browse/JDK-8230776
>>>
>>>
>>> On 8/26/20 4:26 AM, Jan Lahoda wrote:
>>>> Hi Vicente,
>>>>
>>>> I am sorry, but I don't think this is a fix on a good place.
>>>>
>>>> The root cause here is that the classfile for Test has an invalid 
>>>> method descriptor for the Test's constructor (returns float instead 
>>>> of void). See JVMS 4.6., descriptor_index. I think it would be much 
>>>> better to validate method descriptors, and reject classfiles with 
>>>> descriptors not adhering to JVMS. The user would then get a much 
>>>> more helpful error, saying something like:
>>>> ---
>>>> Sub.java:1: error: cannot access Test
>>>> class Sub extends Test {}
>>>>                   ^
>>>>   bad class file: 
>>>> /home/jlahoda/src/jdk/jdk/build/linux-x86_64-server-release/nb-jtreg/work/classes/tools/javac/T8230776/AssertionErrorTest.d/Test.class 
>>>>
>>>>     method descriptor invalid for <init>
>>>>     Please remove or make sure it appears in the correct 
>>>> subdirectory of the classpath.
>>>> 1 error
>>>> ---
>>>>
>>>> What do you think?
>>>>
>>>> Jan
>>>>
>>>> On 25. 08. 20 19:23, Vicente Romero wrote:
>>>>> Please review fix for [1] at [2]. The fix is ending the compilation 
>>>>> at the code generation phase if a statement cant be generated and 
>>>>> there is no way for the compilation to continue.
>>>>>
>>>>> Thanks,
>>>>> Vicente
>>>>>
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8230776
>>>>> [2] http://cr.openjdk.java.net/~vromero/8230776/webrev.00/
>>>
>