[foreign-jextract] [Rev 01] RFR: 8239128: Macro defines constant pointers can crash VM via jextract API

[Maurizio Cimadamore]

On Thu, 20 Feb 2020 21:44:54 GMT, Henry Jen <henryjen at openjdk.org> wrote:

>> The fundamental issue of the crash is that clang cursor/type may no longer valid after reparse another macro, and with pointer type lazily resolve the pointee type, if the clang cursor/type no longer valid, access invalid memory cause the crash.
>> 
>> The other issue is the a macro pointer to a record type like struct/union can cause NPE, and that cause the macro to be ignored on generation without a warning.
>> 
>> The fix remove the laziness from Type API perspective, but leave that as an implementation detail, so implementation of Pointer type need to figure out how to do that safely internally.
>> 
>> Record type in macro are reduces to void, as currently the Declaration implementation is depending on clang as well, we cannot guarantee that works after reparse.
> 
> The pull request has been updated with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase.

Good job - I've left few minor comments, but the bulk of the work seems very solid.

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/Parser.java line 107:

> 106:         Declaration.Scoped rv = treeMaker.createHeader(tuCursor, decls);
> 107:         treeMaker.freeze();
> 108:         return rv;

Does this mean that we can close the index after this?

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/TypeMaker.java line 48:

> 47: 
> 48:     private class ClangTypeReference implements Supplier<Type> {
> 49:         jdk.internal.clang.Type origin;

Nice - I like it; simpler than what I proposed, but equally effective.

src/jdk.incubator.jextract/share/classes/jdk/incubator/jextract/Type.java line 426:

> 425:      */
> 426:     static Type.Function function(boolean varargs, Type returnType, Type... arguments) {
> 427:         return new TypeImpl.FunctionImpl(varargs, Stream.of(arguments).collect(Collectors.toList()), returnType);

This shouldn't be removed! It's the only way for a client to create mutually referring pointer types.

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/TypeMaker.java line 185:

> 184:             case Record: {
> 185:                 if (treeMaker == null) {
> 186:                     // Macro evaluation, type is meaningless as this can only be pointer and we only care value

Do we get a crash if we go here inside macro eval? Why the special casing - I mean I get that we probably don't care, but trying to understand.

test/jdk/java/jextract/JextractApiTestBase.java line 1:

> 1: /*
> 2:  * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.

Thx for splitting this out!

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



PR: https://git.openjdk.java.net/panama-foreign/pull/21