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

runtime/bin/eventhandler_macos.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #if !defined(DART_IO_DISABLED)
 
 #include "platform/globals.h"
 #if defined(TARGET_OS_MACOS)
 
 #include "bin/eventhandler.h"
@@ skipping 25 matching lines @@
 }
 
 
 bool DescriptorInfo::HasWriteEvent() {
   return (Mask() & (1 << kOutEvent)) != 0;
 }
 
 
 // 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];
   int flags = EV_ADD;
   if (!di->IsListeningSocket()) {
     flags |= EV_CLEAR;
   }
 
   ASSERT(di->HasReadEvent() || di->HasWriteEvent());
 
@@ skipping 12 matching lines @@
   int status =
       NO_RETRY_EXPECTED(kevent(kqueue_fd_, events, changes, NULL, 0, NULL));
   if (status == -1) {
     // TODO(dart:io): Verify that the dart end is handling this correctly.
 
     // kQueue does not accept the file descriptor. It could be due to
     // already closed file descriptor, or unuspported devices, such
     // 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);
   }
 }
 
 
 EventHandlerImplementation::EventHandlerImplementation()
     : socket_map_(&HashMap::SamePointerValue, 16) {
   intptr_t result;
   result = NO_RETRY_EXPECTED(pipe(interrupt_fds_));
   if (result != 0) {
     FATAL("Pipe creation failed");
@@ skipping 46 matching lines @@
 
 void EventHandlerImplementation::UpdateKQueueInstance(intptr_t old_mask,
                                                       DescriptorInfo* di) {
   intptr_t new_mask = di->Mask();
   if (old_mask != 0 && new_mask == 0) {
     RemoveFromKqueue(kqueue_fd_, di);
   } else if ((old_mask == 0) && (new_mask != 0)) {
     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.

   }
 }
 
 
 DescriptorInfo* EventHandlerImplementation::GetDescriptorInfo(
     intptr_t fd,
     bool is_listening) {
   ASSERT(fd >= 0);
   HashMap::Entry* entry = socket_map_.Lookup(GetHashmapKeyFromFd(fd),
                                              GetHashmapHashFromFd(fd), true);
@@ skipping 206 matching lines @@
     if ((events[i].flags & EV_ERROR) != 0) {
       const int kBufferSize = 1024;
       char error_message[kBufferSize];
       Utils::StrError(events[i].data, error_message, kBufferSize);
       FATAL1("kevent failed %s\n", error_message);
     }
     if (events[i].udata == NULL) {
       interrupt_seen = true;
     } else {
       DescriptorInfo* di = reinterpret_cast<DescriptorInfo*>(events[i].udata);
       intptr_t event_mask = GetEvents(events + i, di);

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

Unrelated side note: 

Suggestion: We could traverse our data structures here (i.e. socket_map_) and
ensure that [di] is in them (in DEBUG mode).

This events[i].udata pointer was passed as an opaque value to the kernel and
here we get it back. If it got freed in the mean time (which shouldn't happen,
but who knows), then we would have a use-after-free and that could lead to an
issue in UpdateKQueueInstance().

       if ((event_mask & (1 << kErrorEvent)) != 0) {
         di->NotifyAllDartPorts(event_mask);
       }
       event_mask &= ~(1 << kErrorEvent);
 
       if (event_mask != 0) {
         intptr_t old_mask = di->Mask();

[zra, 2017/01/20 15:54:30]

I'm pretty sure the problem is here.

If the event_mask includes an error along with another event, and the
NotifyAllDartPorts on 387 drops the token count to 0, then we have the situation
that the fd is on the kqueue, but this old_mask and the new_mask read by
UpdateKQueueInstance will both be 0, making UdateKQueueInstance a no-op.

The token count has dropped to 0, which means there are a bunch of old messages
from the eventhandler sitting on the application isoalte's queue. When it
finally gets around to processing them, it sends a message to the eventhandler
instructing it to refresh the token count add the fd to the kqueue again. But
it's already there, and we hit the assert.

Without the assert, we're in an odd state for a bit, but the fd is eventually
closed and removed from the kqueue when the Isolate finally gets around to the
message with the error on it. The code here is the same as the code on Linux
etc. so we have the same inconsistency there, but don't notice the problem
because there is no assert.

[kustermann, 2017/01/23 11:49:34]

Very good point.

         Dart_Port port = di->NextNotifyDartPort(event_mask);
         ASSERT(port != 0);
         UpdateKQueueInstance(old_mask, di);
         DartUtils::PostInt32(port, event_mask);
       }
     }
   }
   if (interrupt_seen) {
     // Handle after socket events, so we avoid closing a socket before we handle
     // the current events.
@@ skipping 97 matching lines @@
   // The hashmap does not support keys with value 0.
   return dart::Utils::WordHash(fd + 1);
 }
 
 }  // namespace bin
 }  // namespace dart
 
 #endif  // defined(TARGET_OS_MACOS)
 
 #endif  // !defined(DART_IO_DISABLED)