VM: Fix segfault during shutdown of socket listening registry

VM: Fix segfault during shutdown of socket listening registry

The following happened:

A listening socket (ipv4, first-port) was created.  Afterwards another
listening socket (ipv6, 0) is being created.  We look up in the hashmap
if there is already a listening socket on port `0` (which is never the case).
So we create a new socket, the OS assigns the socket a random port
(which just happens to be `first-port` as well).  Then we insert a new OSSocket
entry into the hashmap with key `first-port` (and set its `next` field to NULL).
This will simply override the existing entry.

During shutdown we loop over all (fd, OSSocket) pairs and try to remove
`OSSocket` from the port->socket map (via the linked `next` list).  We cannot
find the `OSSocket` which we accidentally droped earlier, so we dereference
`0->next` and hit a SEGFAULT.

What should've happend is to correctly link the `OSSocket`s via the next field.

Fixes #27847

R=asiva@google.com, fschneider@google.com

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

[kustermann, 2016-11-29 23:28:40]

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

[kevmoo, 2016-11-29 23:58:36]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2016-11-29 23:58:37]

DBQ: possible to create a regression test for this? Seems like something we
should avoid in the future...

[Florian Schneider, 2016-11-30 00:21:01]

fschneider@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2016-11-30 00:21:02]

Lgtm.

Maybe add "fixes #27847" to the CL description.

@kevmoo: It may not be possible to write a good regression test here, since this
failure depends on the random port number returned from the OS.

https://codereview.chromium.org/2540013002/diff/1/runtime/bin/socket.cc
File runtime/bin/socket.cc (right):

https://codereview.chromium.org/2540013002/diff/1/runtime/bin/socket.cc#newco...
runtime/bin/socket.cc:104: MutexLocker ml(ListeningSocketRegistry::mutex_);
nit: no need to use qualified name here

MutexLocker ml(mutex_);

should be enough?

https://codereview.chromium.org/2540013002/diff/1/runtime/bin/socket.cc#newco...
runtime/bin/socket.cc:167: ASSERT(port == 0);
Maybe add a short comment here about the scenario you described in the CL
description.

[siva, 2016-11-30 00:41:29]

lgtm.

Agree that a reliable regression test here is a problem as the test has to rely
on the port returned by the OS which is random. If we pick a port number then
the test could sometimes fail with 'port already in use' errors if that number
is in use.

[kustermann, 2016-11-30 10:39:39]

Description was changed from

==========
VM: Fix segfault during shutdown of socket listening registry

The following happened:

A listening socket (ipv4, first-port) was created.  Afterwards another
listening socket (ipv6, 0) is being created.  We look up in the hashmap
if there is already a listening socket on port `0` (which is never the case).
So we create a new socket, the OS assigns the socket a random port
(which just happens to be `first-port` as well).  Then we insert a new OSSocket
entry into the hashmap with key `first-port` (and set its `next` field to NULL).
This will simply override the existing entry.

During shutdown we loop over all (fd, OSSocket) pairs and try to remove
`OSSocket` from the port->socket map (via the linked `next` list).  We cannot
find the `OSSocket` which we accidentally droped earlier, so we dereference
`0->next` and hit a SEGFAULT.

What should've happend is to correctly link the `OSSocket`s via the next field.
==========

to

==========
VM: Fix segfault during shutdown of socket listening registry

The following happened:

A listening socket (ipv4, first-port) was created.  Afterwards another
listening socket (ipv6, 0) is being created.  We look up in the hashmap
if there is already a listening socket on port `0` (which is never the case).
So we create a new socket, the OS assigns the socket a random port
(which just happens to be `first-port` as well).  Then we insert a new OSSocket
entry into the hashmap with key `first-port` (and set its `next` field to NULL).
This will simply override the existing entry.

During shutdown we loop over all (fd, OSSocket) pairs and try to remove
`OSSocket` from the port->socket map (via the linked `next` list).  We cannot
find the `OSSocket` which we accidentally droped earlier, so we dereference
`0->next` and hit a SEGFAULT.

What should've happend is to correctly link the `OSSocket`s via the next field.

Fixes #27847
==========

[kustermann, 2016-11-30 10:48:13]

Description was changed from

==========
VM: Fix segfault during shutdown of socket listening registry

The following happened:

A listening socket (ipv4, first-port) was created.  Afterwards another
listening socket (ipv6, 0) is being created.  We look up in the hashmap
if there is already a listening socket on port `0` (which is never the case).
So we create a new socket, the OS assigns the socket a random port
(which just happens to be `first-port` as well).  Then we insert a new OSSocket
entry into the hashmap with key `first-port` (and set its `next` field to NULL).
This will simply override the existing entry.

During shutdown we loop over all (fd, OSSocket) pairs and try to remove
`OSSocket` from the port->socket map (via the linked `next` list).  We cannot
find the `OSSocket` which we accidentally droped earlier, so we dereference
`0->next` and hit a SEGFAULT.

What should've happend is to correctly link the `OSSocket`s via the next field.

Fixes #27847
==========

to

==========
VM: Fix segfault during shutdown of socket listening registry

The following happened:

A listening socket (ipv4, first-port) was created.  Afterwards another
listening socket (ipv6, 0) is being created.  We look up in the hashmap
if there is already a listening socket on port `0` (which is never the case).
So we create a new socket, the OS assigns the socket a random port
(which just happens to be `first-port` as well).  Then we insert a new OSSocket
entry into the hashmap with key `first-port` (and set its `next` field to NULL).
This will simply override the existing entry.

During shutdown we loop over all (fd, OSSocket) pairs and try to remove
`OSSocket` from the port->socket map (via the linked `next` list).  We cannot
find the `OSSocket` which we accidentally droped earlier, so we dereference
`0->next` and hit a SEGFAULT.

What should've happend is to correctly link the `OSSocket`s via the next field.

Fixes #27847

R=asiva@google.com, fschneider@google.com

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

[kustermann, 2016-11-30 10:48:15]

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

[kustermann, 2016-11-30 10:51:11]

@kevmoo: As florian/siva already mentioned, this is an issue which only occurs
precisely if we allow the kernel to choose a new random port for a listening
socket and that very same port is already being listened upon by dart code (on a
different address).

https://codereview.chromium.org/2540013002/diff/1/runtime/bin/socket.cc
File runtime/bin/socket.cc (right):

https://codereview.chromium.org/2540013002/diff/1/runtime/bin/socket.cc#newco...
runtime/bin/socket.cc:104: MutexLocker ml(ListeningSocketRegistry::mutex_);
On 2016/11/30 00:21:01, Florian Schneider wrote:
> nit: no need to use qualified name here
> 
> MutexLocker ml(mutex_);
> 
> should be enough?

Done.

https://codereview.chromium.org/2540013002/diff/1/runtime/bin/socket.cc#newco...
runtime/bin/socket.cc:167: ASSERT(port == 0);
On 2016/11/30 00:21:01, Florian Schneider wrote:
> Maybe add a short comment here about the scenario you described in the CL
> description.

Done.