VM eventhandler: Read "old" event mask before it can be modified.

runtime/bin/eventhandler_macos.cc

Index: runtime/bin/eventhandler_macos.cc
diff --git a/runtime/bin/eventhandler_macos.cc b/runtime/bin/eventhandler_macos.cc
index 640f05a5408521b9287fbcff029b918417fe5bff..1094e5cad441b96323d4772b58cf280a4da7d728 100644
--- a/runtime/bin/eventhandler_macos.cc
+++ b/runtime/bin/eventhandler_macos.cc
@@ -43,23 +43,18 @@ bool DescriptorInfo::HasWriteEvent() {
 
 // Unregister the file descriptor for a SocketData structure with kqueue.
 static void RemoveFromKqueue(intptr_t kqueue_fd_, DescriptorInfo* di) {
-  if (!di->tracked_by_kqueue()) {
-    return;
-  }
   static const intptr_t kMaxChanges = 2;
   struct kevent events[kMaxChanges];

[kustermann, 2017/01/20 12:22:49]

Unrelated side note:

Seems like legacy code: We only use events[0] and never [1].

[zra, 2017/01/20 17:16:32]

Maybe in another CL. We should try the bug fix in isolation.

   EV_SET(events, di->fd(), EVFILT_READ, EV_DELETE, 0, 0, NULL);
   VOID_NO_RETRY_EXPECTED(kevent(kqueue_fd_, events, 1, NULL, 0, NULL));

[kustermann, 2017/01/20 12:22:49]

Unrelated side note: 

We should probably check the return value. I assume the reason we don't check it
is that we just remove "read | write" even though we might have only setup one
of the two.

We should know from [di] what we were interested in before, and could just
perform the remove that we need. This will allow us to also validate the return
code.

[zra, 2017/01/20 17:16:32]

ditto

   EV_SET(events, di->fd(), EVFILT_WRITE, EV_DELETE, 0, 0, NULL);
   VOID_NO_RETRY_EXPECTED(kevent(kqueue_fd_, events, 1, NULL, 0, NULL));

[kustermann, 2017/01/20 12:22:49]

Unrelated side note: 

As opposed to the adding code, we do 2 syscalls here instead of one syscall with
2 changes further down.

[zra, 2017/01/20 17:16:32]

ditto

-  di->set_tracked_by_kqueue(false);
 }
 
 
 // Update the kqueue registration for SocketData structure to reflect
 // the events currently of interest.
 static void AddToKqueue(intptr_t kqueue_fd_, DescriptorInfo* di) {
-  ASSERT(!di->tracked_by_kqueue());

[kustermann, 2017/01/20 12:22:49]

I cannot think of a case where this ASSERT should be hit. So I assume therefore
that something goes wrong.

This boolean is entirely controlled by RemoveFromKqueue()/AddToKQueue(). Which
are in return entirely controlled by UpdateKQueueInstance(). Which guarantees
removing before adding.

   static const intptr_t kMaxChanges = 2;
   intptr_t changes = 0;
   struct kevent events[kMaxChanges];
@@ -92,8 +87,6 @@ static void AddToKqueue(intptr_t kqueue_fd_, DescriptorInfo* di) {
     // as /dev/null. In such case, mark the file descriptor as closed,
     // so dart will handle it accordingly.
     di->NotifyAllDartPorts(1 << kCloseEvent);
-  } else {
-    di->set_tracked_by_kqueue(true);
   }
 }
 
@@ -160,7 +153,8 @@ void EventHandlerImplementation::UpdateKQueueInstance(intptr_t old_mask,
     AddToKqueue(kqueue_fd_, di);
   } else if ((old_mask != 0) && (new_mask != 0) && (old_mask != new_mask)) {
     ASSERT(!di->IsListeningSocket());
-    RemoveFromKqueue(kqueue_fd_, di);
+    // kevent() will automatically replace the old filter for an existing fd.
+    // There is no need to remove it first.
     AddToKqueue(kqueue_fd_, di);

[kustermann, 2017/01/20 12:22:49]

This is an incorrect change if I understand it properly:

The kevent() call will process a list of (ident, filter, [options]) changes. If
there is already an (ident, filter) setup, then it is replaced.

BUT:

AddToKqueue() will only handle 0 -> 1 changes in the mask (it will be a NOP for
1 -> 0 changes in the mask).

Example scenario where it goes wrong:

  old_mask = read | write;
  new_mask = write;

  call AddToKQueue(di):
     kevent(kq, [(fd, EVFILT_WRITE, [EV_ADD, di])]

This keeps the old (fd, EVFILT_READ) registration in. Which means we will
continue to get events from the kernel when data arrives **even though we do not
want to**!

[zra, 2017/01/20 17:16:32]

Acknowledged.

   }
 }