OpenSSL and panama-foreign

[Maurizio Cimadamore]

Looking more at your code, I noticed you use cleaners:

https://github.com/rmaucher/openssl-panama-foreign/blob/5869df2c2e58a0a4aa8e0883c1a896cd49e96146/src/main/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLEngine.java#L275

That is, when the engine is unreacheable, you have some cleanup actions:

https://github.com/rmaucher/openssl-panama-foreign/blob/5869df2c2e58a0a4aa8e0883c1a896cd49e96146/src/main/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLEngine.java#L1868

Note that you can probably achieve the same effect by registering the 
scope with a cleaner, and then add a "close action" on the cleaner 
itself, to close the scope. But this is an optional suggestion.

What I find more disturbing is that the addresses you generate, e.g. this:

https://github.com/rmaucher/openssl-panama-foreign/blob/5869df2c2e58a0a4aa8e0883c1a896cd49e96146/src/main/java/org/apache/tomcat/util/openssl/openssl_h.java#L1327

This `ssl` address is just a raw memory address - it doesn't have any 
real temporal bounds. It is not attached to the scope you create in any way.

So, when you do this:

https://github.com/rmaucher/openssl-panama-foreign/blob/5869df2c2e58a0a4aa8e0883c1a896cd49e96146/src/main/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLEngine.java#L766

The linker won't really protect that call in any way. There's no way for 
the linker to know whether the address is still available or not. Which 
might be ok if the memory is managed by SSL anyway.

But the issue here, as your debugging session shows, is that, somehow, 
you end up calling SSL_shutdown on a bad SSL pointer (e.g. NULL). Which 
looks like a client error (e.g. how can SSL be NULL ? )

Maurizio




On 10/11/2021 09:34, Maurizio Cimadamore wrote:
> Hi,
> thanks for sharing the feedback! I'm glad you got something working. 
> Hopefully we can fix the rest :-)
>
> On 10/11/2021 09:20, Rémy Maucherat wrote:
>> However, I am running into cores (apparently caused by memory
>> corruption) with the panama-foreign branch, while the Java 17 version
>> seems solid.
>
> That is interesting information. So, you say that the Java 17 version 
> works fine, but when building (latest) panama-foreign, you get some VM 
> crash.
>
> Now, looking at the crash, it seems like it occurs in the middle of a 
> native call:
>
> ```
> Java frames: (J=compiled Java code, j=interpreted, Vv=VM code)
> v  ~RuntimeStub::nep_invoker_blob <------------
> J 5738 c1 
> org.apache.tomcat.util.openssl.openssl_h.SSL_shutdown(Ljdk/incubator/foreign/Addressable;)I 
> (31 bytes) @ 0x00007faf82340d2c [0x00007faf8233cc20+0x000000000000410c]
> J 5482 c1 
> org.apache.tomcat.util.net.openssl.panama.OpenSSLEngine.closeOutbound()V 
> (76 bytes) @ 0x00007faf81a934e4 [0x00007faf81a93280+0x0000000000000264]
> ```
>
> This area changed quite a bit recently, as we are refactoring and 
> consolidating the linking functionalities to make it simpler for 
> developers to port the CLInker implementation to other platforms. I 
> wonder if a regression sneaked in (possible, the refactoring were 
> quite big).
>
> In terms of support for shared scopes, the one thing that has changed 
> from 16 to 17 is that now if you pass arguments by reference to 
> functions, those references' scopes are "acquired" on function enter 
> and "released" on function exit - which means it is not possible, even 
> in a shared context, for e.g. the target address of a native function 
> to be unloaded in the middle of a native call. But this should add 
> _more_ safety, not less.
>
> I suppose that you tested Java 17 by using the jextract available in 
> the Panama EA binaries; and that you tested the bits in panama-foreign 
> by manually building them, and using the jextract you obtained as a 
> result (e.g. using the EA jextract against the latest panama-foreign 
> API will NOT work).
>
> Thanks
> Maurizio
>