OpenSSL and panama-foreign

[Jorn Vernee]

Looking at this, it seems there is a race between the cleaner running 
and the SSL_shutdown call (i.e. undefined behaviour). Note that the 
`this` object (which the cleanup action is registered on) can be 
collected as soon as it's unused in closeOutbound, and SSL_shutdown 
doesn't use it.

You'd at least need `Reference.reachabilityFence(this)` after the call 
to SSL_shutdown.

But, I'd suggest doing what Maurizio suggested instead. In the 
constructor of OpenSSLEngine:

- Use ResourceScope.newImplicitScope() instead to create the scope [1]
- Attach both 'ssl' and 'networkBIO' to the scope by turning them into 
memory segments using MemorySegment::ofAddressNative [2]
- Then add a close action to the scope which frees your pointers [3]

The call will keep the scope alive, which makes sure the cleanup isn't 
done during, or before, the native call.

HTH,
Jorn

[1] : 
https://github.com/openjdk/panama-foreign/blob/foreign-jextract/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java#L247
[2] : 
https://github.com/openjdk/panama-foreign/blob/foreign-jextract/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java#L791
[3] : 
https://github.com/openjdk/panama-foreign/blob/foreign-jextract/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java#L190

On 10/11/2021 13:53, Maurizio Cimadamore wrote:
> 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
>>