RFR: 8268894: forged ASTs can provoke an AIOOBE at com.sun.tools.javac.jvm.ClassWriter::writePosition

[Guoxiong Li]

On Fri, 27 Aug 2021 20:32:00 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> Hi all,
>> 
>> The method `TypeAnnotationPosition.updatePosOffset` sets the `lvarOffset` unnecessarily. After invoking `TypeAnnotationPosition.updatePosOffset`, the lengths of (`lvarOffset`, `lvarLength`, `lvarIndex`) are respectively (1, 0, 0). Then `Code.fillLocalVarPosition` fills the `lvarOffset`, `lvarLength`, `lvarIndex` which make their lengths become (N, N-1, N-1). At last, the method `ClassWrite.writePosition` uses these three arrays whose lengths are not the same so that the `ArrayIndexOutOfBoundsException` occurs.
>> 
>> The `lvarOffset` should be always changed along with `lvarLength` and `lvarIndex` so that their lengths can be the same.
>> 
>> This patch removes the unnecessary `lvarOffset = new int[]{to};` of the method  `TypeAnnotationPosition.updatePosOffset` and add a corresponding test.
>> 
>> ---
>> Steps to reproduce the issue:
>> 
>> 1. Construct a annotation processor which set the `pos` of the method sub-tree nodes to the same position. Please see the file `TypeAnnotationPositionProcessor.java` for more details.
>> 2. Compile the code which has the corresponding method with the annotated local variable by using the annotation processor generated at the step 1. Please see the file `TypeAnnotationPositionTest.java` for more details.
>> 3. Then the crash occurs.
>> 
>> Actually, this bug may never occur if the users use the compiler friendly. Because the method `TypeAnnotationPosition.updatePosOffset`, which we mentioned at the beginning, may never be invoked at the situation that the different trees has different and right position.
>> 
>> The lib `lombok` constructs some new tree nodes by using the internal method of the compiler. But it doesn't set the `pos` correctly and uses the same fake position. The unfriendly code of the `lombok` is in the method [JavacHandlerUtil.setGeneratedBy](https://github.com/projectlombok/lombok/blob/master/src/core/lombok/javac/handlers/JavacHandlerUtil.java#L167). At much time, the fake same position works well, but this time, it crashes the compiler.
>> 
>> ---
>> A brief history:
>> 
>> According to the git's records, I found the code `ta.position.lvarOffset = new int[] { code.cp };` was firstly added at [8006775: JSR 308: Compiler changes in JDK8](http://hg.openjdk.java.net/jdk8/jdk8/langtools/rev/71f35e4b93a5#l52.35) .
>> 
>> Then, [8013852: update reference impl for type-annotations](http://hg.openjdk.java.net/jdk8/jdk8/langtools/rev/ddb4a2bfcd82#l8.52) moved the code to `TypeAnnotationPosition.updatePosOffset`.
>> 
>> The bug didn't occur at that time, because the old [Code.fillLocalVarPosition](http://hg.openjdk.java.net/jdk8/jdk8/langtools/rev/71f35e4b93a5#l51.21) always created a new array instead of using the old array.
>> 
>> But later, [8231826: Implement javac changes for pattern matching for instanceof](https://hg.openjdk.java.net/jdk/jdk/rev/7799a51dbe30#l24.27) revised the method `Code.fillLocalVarPosition` to reuse the old array so that the bug occurs.
>> 
>> Concluded, this is an issue introduced at JDK8 when implementing the type annotation. But it was hidden by other code at the past. 
>> 
>> ---
>> 
>> Thanks for taking the time to review.
>> 
>> Best Regards,
>> -- Guoxiong
>
> please update the PR title with the last bug title: "8268894: forged ASTs can provoke an AIOOBE at com.sun.tools.javac.jvm.ClassWriter::writePosition" or please sync both to another version you like better before pushing

@vicente-romero-oracle Thanks for your review.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4749