OpenSSL and panama-foreign

[Rémy Maucherat]

On Thu, Nov 11, 2021 at 2:53 AM Maurizio Cimadamore
<maurizio.cimadamore at oracle.com> wrote:
>
> I think I managed to figure out what introduced the regression.
>
> Basically, it's down to the removal of customization for upcall stubs.
>
> ```
>
> @@ -123,7 +120,7 @@ public static UpcallHandler make(ABIDescriptor abi, MethodHandle target, Calling
>                  .anyMatch(s -> abi.arch.isStackType(s.type()));
>          if (USE_INTRINSICS && isSimple && !usesStackArgs && supportsOptimizedUpcalls()) {
>              checkPrimitive(doBindings.type());
> -            JLI.ensureCustomized(doBindings);
> +            doBindings = insertArguments(exactInvoker(doBindings.type()), 0, doBindings);
>              VMStorage[] args = Arrays.stream(argMoves).map(Binding.Move::storage).toArray(VMStorage[]::new);
>              VMStorage[] rets = Arrays.stream(retMoves).map(Binding.Move::storage).toArray(VMStorage[]::new);
>              CallRegs conv = new CallRegs(args, rets);
>
> ```

One line fixes are the best !

>
> If this patch is reverted, then everything works as expected (I've run your `ab` test 10 consecutive times, no crashes). Which seems like a latent bug in the upcall machinery.
>
> That said, I think that, for the time being, it would be best to revert this code to what it was in 17, to avoid these spurious crashes.
>
> I wonder if the reason behind the issue we're seeing is caused by the Addressable vs. MemoryAddress mismatch in the method type - e.g. maybe `exactInvoker(doBindings.type())` doesn't do what we think it should. I'll keep investigating tomorrow (well, later today :-) ).
>

That reminds me of some upcall trouble I had with return types. Until
recently on panama-foreign, I had to use long for non primitive types
(a hack to fit a pointer, it worked ...) since nothing else was
working. Now I can use Addressable. With Java 17, MemoryAddress works
fine.

> 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)
>

Unless I missed something the API has a side effect with upcalls. I
understand after reading the javadoc where the suggestion comes from,
as it makes memory management very much like Java. This is of course
excellent. However, I use upcalls (as you know) and many of them are
bound to an instance (since this makes state tracking very very nice).
This however creates a reference that never goes away and an implicit
scope is never going to get GCed and closed (I saw that with visualvm)
and memory leaks. Initially I was planning to use a static Map<Long,
State> for my state (OpenSSL allows passing references and getting
them in callbacks, but this is a lot handier in C and doesn't make a
lot of sense in Java), then I found MethodHandle.bindTo actually
worked and this is quite magical.

So with that in mind, what is the recommendation ? Should the API
provide a way to remove the MethodHandle upcall ? Or should I avoid
using bindTo instead ?

Rémy



Rémy

> Cheers
> Maurizio
>
> On 10/11/2021 22:38, Maurizio Cimadamore wrote:
>
> And, on this same vein, even with the resource scope handshake disabled, Tomcat still crashes with the following Java option:
>
> export JAVA_OPTS="-XX:+DeoptimizeALot --enable-native-access=ALL-UNNAMED --add-modules jdk.incubator.foreign"
>
> DeoptimizeALot causes frequent deopt events in the VM. The handshake we use for shared segments also used deopt events. Something got broken here and the upcall intrinsic doesn't seem to be able to handle deopt events.
>
> Maurizio
>
>
> On 10/11/2021 22:27, Maurizio Cimadamore wrote:
>
> This seems to point at some issues with the shared scope logic interacting badly with upcall intrinsics. But neither areas have changed significantly since 17, which makes this very odd.