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

[Henry Jen]

On Mon, 17 Feb 2020 13:49:57 GMT, Maurizio Cimadamore <mcimadamore 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.
> 
> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/MacroParserImpl.java line 100:
> 
>> 99:                 .findAny().get();
>> 100:         typeMaker.resolveTypeReferences();
>> 101:         return rv;
> 
> Should this call go here? Probably - if we drop the toplevel unit after subsequent reparse. But, I think we should also put a call on this at the end of parsing? Let's say I have  something like:
> 
> struct A {
>    struct B *b;
> }
> 
> struct B {
>   struct A *a;
> }
> 
> No macros here - how does this work? I believe you will add pointers in the todo list (e.g. when we parse `struct A` we will add `B*`). Who resolves that? And, if we did a resolve, wouldn't that end up adding another pointer (`A*`) to the list? Who would resolve that? And if we re-run resolve on the new pointer, how do we prevent the logic from re-adding the already resolved pointer type `B*` ?

resolveTypeReferences suppose to be called after parse or reparse, so we no longer depends on clang type.

Theoretically, we also should call resolveTypeReference after parse for non-macro case, but as I noted in the code, we haven't completely get rid of dependency on cursor for declaration API, so we now still have to operate under an valid clang TU for non-macro case, thus there is no need to call that just yet. Because when type() is called on the pointer, the resolve happens automatically.

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

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