OpenSSL and panama-foreign

[Maurizio Cimadamore]

I see what you mean - e.g. this code is just a straight port of the JNI 
code.

That said, it's possible that the JNI code kept alive some Java objects 
via JNI handles - which with Panama would no longer be true.

I'd recommend to double check if, when you get a core dump, your cleanup 
actions are run prematurely. Right now it's a bit hard to tell given 
that some address are raw native addresses (w/o a lifecycle) and some do 
have a lifecycle (scope).

But looking at your debug for the crash you got on SSL_Shutdown, I still 
don't get why SSL* is set to zeros (NULL). I don't think you are 
assigning that pointer anywhere else (OpenSSLState.ssl is final).

Which seems to suggest that, somehow, that ssl pointer is set to NULL 
from the start, and then you get a crash when you try to do anything 
with it.

But I agree that it looks suspicious that the behavior changed from 17.

I'd suggest starting from making sure that there's just one cleanup 
mechanism (the scope). E.g.

1. create an implicit scope, as suggested by Jorn instead of a shared one.
2. do not register anything with a cleaner. Instead add a cleanup 
action, so that when the scope is closed (implicitly, by a cleaner), you 
call BIO_free and SSL_free on the pointers you have.
3. I think you might need to add reachability fences - because there's 
no guarantee that the scope would be kept reachable while you call 
methods on SSLEngine - the VM might detect that `this` is no longer used 
and then reclaim it (as Jorn mentioned, I'm not sure if relying on 
synchronized blocks is enough for this)

In other words, let's simplify the lifecycle management a bit, so that 
we can rule out e.g. that the scope is close _while_ one of the native 
call is taking place. If we do that, and we still get a crash, that 
might point to a real regression - as things are, it's hard to tell - it 
might just have been a latent bug which now became more manifest.

Maurizio


On 10/11/2021 14:54, Rémy Maucherat wrote:
> Thanks a lot for the great design ideas. Keeping around MemorySegment
> instead of MemoryAddress is a much better design. I'll also use the
> scope as the controller for closing everything.
>
> Notes: The cleaner comes from a Java 11 finalize refactoring. The sync
> model was also there before with the JNI code, it worked. Overall this
> is a translation of the JNI calling code, the ssl there was simply a
> long (so not very safe ...).