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

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

related #24417

R=johnmccutchan@google.com, kustermann@google.com

Committed: https://github.com/dart-lang/sdk/commit/e2220c3ea88844a82f075a4a07623780f49c8dd3

[zra, 2017-01-17 17:07:01]

zra@google.com changed reviewers:
+ johnmccutchan@google.com, kustermann@google.com

[Cutch, 2017-01-17 17:33:01]

RS LGTM

[kustermann, 2017-01-17 17:51:26]

@zach: If you are not in a hurry with this, please hold it off. When I find some
time I'll take a look at the code again. I suppose the assert was there for a
reason.

[zra, 2017-01-17 17:54:34]

On 2017/01/17 17:51:26, kustermann wrote:
> @zach: If you are not in a hurry with this, please hold it off. When I find
some
> time I'll take a look at the code again. I suppose the assert was there for a
> reason.

It looks like the failure is pretty rare, so I don't think there's any hurry.

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

Not 100% sure if I'm reading the kevent() man page correctly, but I think this
CL is actually not quite correct (we don't handle the (1 -> 0 changes in the
mask anymore if I see this correctly).

I left a few notes (unrelated to your CL) while reading through this code.


Since I think the ASSERT() is valid and we should never hit it, I would not
remove it.

We could however do 2 things:

  * validate that the "events[i].udata" we get from the back is valid (we have
it in the socket_map_)
  * check the return value of kevent() when doing EV_DELETE (and only EV_DELETE
the ones we registered before)

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
File runtime/bin/eventhandler_macos.cc (left):

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:62: ASSERT(!di->tracked_by_kqueue());
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.

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
File runtime/bin/eventhandler_macos.cc (right):

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:47: struct kevent events[kMaxChanges];
Unrelated side note:

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

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:49: VOID_NO_RETRY_EXPECTED(kevent(kqueue_fd_,
events, 1, NULL, 0, NULL));
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.

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:51: VOID_NO_RETRY_EXPECTED(kevent(kqueue_fd_,
events, 1, NULL, 0, NULL));
Unrelated side note: 

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

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:158: AddToKqueue(kqueue_fd_, di);
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**!

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:385: intptr_t event_mask = GetEvents(events +
i, di);
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().

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

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
File runtime/bin/eventhandler_macos.cc (right):

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:392: intptr_t old_mask = di->Mask();
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.

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

The fix is to read the old_mask before it might be modified by sending the
notification that decrements the token count.

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
File runtime/bin/eventhandler_macos.cc (right):

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:47: struct kevent events[kMaxChanges];
On 2017/01/20 12:22:49, kustermann wrote:
> Unrelated side note:
> 
> Seems like legacy code: We only use events[0] and never [1].

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

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:49: VOID_NO_RETRY_EXPECTED(kevent(kqueue_fd_,
events, 1, NULL, 0, NULL));
On 2017/01/20 12:22:49, kustermann wrote:
> 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.

ditto

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:51: VOID_NO_RETRY_EXPECTED(kevent(kqueue_fd_,
events, 1, NULL, 0, NULL));
On 2017/01/20 12:22:49, kustermann wrote:
> Unrelated side note: 
> 
> As opposed to the adding code, we do 2 syscalls here instead of one syscall
with
> 2 changes further down.

ditto

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:158: AddToKqueue(kqueue_fd_, di);
On 2017/01/20 12:22:49, kustermann wrote:
> 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**!

Acknowledged.

[kustermann, 2017-01-23 11:47:59]

LGTM

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

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
File runtime/bin/eventhandler_macos.cc (right):

https://codereview.chromium.org/2635253002/diff/1/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:392: intptr_t old_mask = di->Mask();
On 2017/01/20 15:54:30, zra wrote:
> 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.

Very good point.

[zra, 2017-01-23 16:46:11]

Description was changed from

==========
Remove assert from the mac eventhandler.

Also remove unneeded kevent remove.

related #24417
==========

to

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

related #24417
==========

[zra, 2017-01-23 16:54:56]

Description was changed from

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

related #24417
==========

to

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

related #24417

R=johnmccutchan@google.com, kustermann@google.com

Committed:
https://github.com/dart-lang/sdk/commit/e2220c3ea88844a82f075a4a07623780f49c8dd3
==========

[zra, 2017-01-23 16:54:58]

Committed patchset #2 (id:20001) manually as
e2220c3ea88844a82f075a4a07623780f49c8dd3 (presubmit successful).