From 1df8f2ccd1f12a7b5c455d7b8e44d5b28fa4be7c Mon Sep 17 00:00:00 2001
From: Scott Mitchell <scott_mitchell@apple.com>
Date: Wed, 28 Jun 2017 14:03:47 -0400
Subject: [PATCH] KQueue crash due to close/cleanup sequencing

Motivation:
The kqueue documentation states that 'Calling close() on a file descriptor will remove any kevents that reference the descriptor.' [1], but doesn't mention if this cleanup will be done synchronously. Under some circumstances it has been observed that cleanup was not done immediately and when KQueueEventLoop attempted to access the channel associated with the event the JVM would crash, a ClassCastException, or generally undefined behavior would occur because of invalid pointer references.

[1] https://www.freebsd.org/cgi/man.cgi?query=kqueue&sektion=2

Modifications:
- AbstractKqueueChannel#doClose should not rely upon this assumption and instead should call doDeregister() to ensure cleanup is done synchronously.
- Deleting a kevent should also set the jniSelfPtr stored in the udata of that kevent to NULL, to ensure we will not dereference it later.

Result:
No more kqueue crash due to close/cleanup sequencing.
---
 .../src/main/c/netty_kqueue_eventarray.c                   | 3 +++
 .../io/netty/channel/kqueue/AbstractKQueueChannel.java     | 7 ++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/transport-native-kqueue/src/main/c/netty_kqueue_eventarray.c b/transport-native-kqueue/src/main/c/netty_kqueue_eventarray.c
index 524bbfbc7a..ad7a20d654 100644
--- a/transport-native-kqueue/src/main/c/netty_kqueue_eventarray.c
+++ b/transport-native-kqueue/src/main/c/netty_kqueue_eventarray.c
@@ -32,6 +32,9 @@ static void netty_kqueue_eventarray_evSet(JNIEnv* env, jclass clzz, jlong kevent
     if (jniSelfPtr == 0) {
         jniSelfPtr = (jlong) (*env)->NewGlobalRef(env, channel);
         (*env)->SetLongField(env, channel, kqueueJniPtrFieldId, jniSelfPtr);
+    } else if ((flags & EV_DELETE) != 0) {
+        // If the event is deleted, make sure it no longer has a reference to the jniSelfPtr because it shouldn't be used after this point.
+        jniSelfPtr = 0;
     }
     EV_SET((struct kevent*) keventAddress, ident, filter, flags, fflags, 0, (jobject) jniSelfPtr);
 }
diff --git a/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/AbstractKQueueChannel.java b/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/AbstractKQueueChannel.java
index cf221798f7..a26bb906fa 100644
--- a/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/AbstractKQueueChannel.java
+++ b/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/AbstractKQueueChannel.java
@@ -98,11 +98,12 @@ abstract class AbstractKQueueChannel extends AbstractChannel implements UnixChan
         // Even if we allow half closed sockets we should give up on reading. Otherwise we may allow a read attempt on a
         // socket which has not even been connected yet. This has been observed to block during unit tests.
         inputClosedSeenErrorOnRead = true;
-        // The FD will be closed, which will take of deleting from kqueue.
-        readFilterEnabled = writeFilterEnabled = false;
         try {
             if (isRegistered()) {
-                ((KQueueEventLoop) eventLoop()).remove(this);
+                // The FD will be closed, which should take care of deleting any associated events from kqueue, but
+                // since we rely upon jniSelfRef to be consistent we make sure that we clear this reference out for all]
+                // events which are pending in kqueue to avoid referencing a deleted pointer at a later time.
+                doDeregister();
             }
         } finally {
             socket.close();