Guard socket file descriptor in UDPSocketLibevent

Guard socket file descriptor in UDPSocketLibevent

Now UDPSocketLibevent enables file descriptor guard for all UDP sockets
to make sure they are not closed from anywhere else except
UDPSocketLibevent::Close(). This is necessary to debug the linked bug

BUG=461246
Committed: https://crrev.com/509aba26b3c3d65cebb1647cb03d8ad1db10eb28
Cr-Commit-Position: refs/heads/master@{#323624}

[Sergey Ulanov, 2015-04-01 22:39:28]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2015-04-01 22:41:55]

sergeyu@chromium.org changed reviewers:
+ mark@chromium.org

[Sergey Ulanov, 2015-04-01 22:41:56]

This doesn't generate crash reports, but I see the following message from
Crashpad:

[0401/151214:WARNING:exception_snapshot_mac.cc(71)] exception_code_0
0x4000000100000065 out of range
Killed: 9

[Mark Mentovai, 2015-04-02 02:00:01]

Can you patch https://codereview.chromium.org/1050313003 into crashpad and let
me know if it starts generating crash reports?

[Mark Mentovai, 2015-04-02 14:37:06]

https://codereview.chromium.org/1053873003/diff/20001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/1053873003/diff/20001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:61: // to be defined here.
Drive the point home by saying in the comment that this is temporary and will be
removed.

https://codereview.chromium.org/1053873003/diff/20001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:66: return syscall(442, fd, guard);
This is only available in 10.9 and 10.10. It’ll be ENOSYS elsewhere. The
majority of users are on 10.9 and 10.10, so this shouldn’t be a problem.

I recommend calling through the syscall wrappers in libSystem instead of making
the syscall() like this. You can dlsym() for "guarded_close_np" and
"change_fdguard_np". If it returns nullptr for either of those, you’ll know that
the kernel doesn’t provide file descriptor guards, and you can fall back to
using close() instead of guarded_close_np().

[Sergey Ulanov, 2015-04-02 17:36:51]

On 2015/04/02 02:00:01, Mark Mentovai wrote:
> Can you patch https://codereview.chromium.org/1050313003 into crashpad and let
> me know if it starts generating crash reports?

I do see crash reports generated with this patch. Thanks!

[Mark Mentovai, 2015-04-02 17:42:39]

OK, I’ll try to get this into Crashpad and roll Chrome to an updated Crashpad
quickly.

What’s the plan for this patch? The bug is labeled ReleaseBlock-Stable and has
both M-42 and M-43. Where will we be collecting data?

[Sergey Ulanov, 2015-04-02 18:05:43]

https://codereview.chromium.org/1053873003/diff/20001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/1053873003/diff/20001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:61: // to be defined here.
On 2015/04/02 14:37:06, Mark Mentovai wrote:
> Drive the point home by saying in the comment that this is temporary and will
be
> removed.

Added TODO to remove it.

https://codereview.chromium.org/1053873003/diff/20001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:66: return syscall(442, fd, guard);
On 2015/04/02 14:37:06, Mark Mentovai wrote:
> This is only available in 10.9 and 10.10. It’ll be ENOSYS elsewhere. The
> majority of users are on 10.9 and 10.10, so this shouldn’t be a problem.
> 
> I recommend calling through the syscall wrappers in libSystem instead of
making
> the syscall() like this. You can dlsym() for "guarded_close_np" and
> "change_fdguard_np". If it returns nullptr for either of those, you’ll know
that
> the kernel doesn’t provide file descriptor guards, and you can fall back to
> using close() instead of guarded_close_np().

Done.

[Sergey Ulanov, 2015-04-02 18:09:21]

On 2015/04/02 17:42:39, Mark Mentovai wrote:
> OK, I’ll try to get this into Crashpad and roll Chrome to an updated Crashpad
> quickly.
> 
> What’s the plan for this patch? The bug is labeled ReleaseBlock-Stable and has
> both M-42 and M-43. Where will we be collecting data?

We get crash reports for this issue from canary as well (around 5 per day), so
we may get to the root cause from the canary crashes, and won't need to merge
this patch to M42.

[Mark Mentovai, 2015-04-02 18:21:57]

Awesome, Sergey. LGTM with these changes.

https://codereview.chromium.org/1053873003/diff/40001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/1053873003/diff/40001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:80: void* g_libsystem_handle = nullptr;
This is only used in InitGuardedFunctions, make it a static void* in that
function.

https://codereview.chromium.org/1053873003/diff/40001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:84: static pthread_once_t
g_guarded_functions_once = PTHREAD_ONCE_INIT;
You’re in an unnamed namespace, you shouldn’t have static on this or on the
function that follows.

https://codereview.chromium.org/1053873003/diff/40001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:87: g_libsystem_handle =
dlopen("/usr/lib/libSystem.dylib", 0);
Use RTLD_LAZY | RTLD_LOCAL | RTLD_NOLOAD, not 0.

[Sergey Ulanov, 2015-04-02 18:28:19]

https://codereview.chromium.org/1053873003/diff/40001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/1053873003/diff/40001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:80: void* g_libsystem_handle = nullptr;
On 2015/04/02 18:21:57, Mark Mentovai wrote:
> This is only used in InitGuardedFunctions, make it a static void* in that
> function.

Done. It doesn't need to be static.

https://codereview.chromium.org/1053873003/diff/40001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:84: static pthread_once_t
g_guarded_functions_once = PTHREAD_ONCE_INIT;
On 2015/04/02 18:21:57, Mark Mentovai wrote:
> You’re in an unnamed namespace, you shouldn’t have static on this or on the
> function that follows.

Done.

https://codereview.chromium.org/1053873003/diff/40001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:87: g_libsystem_handle =
dlopen("/usr/lib/libSystem.dylib", 0);
On 2015/04/02 18:21:57, Mark Mentovai wrote:
> Use RTLD_LAZY | RTLD_LOCAL | RTLD_NOLOAD, not 0.

Done.

[Sergey Ulanov, 2015-04-02 18:28:48]

sergeyu@chromium.org changed reviewers:
+ rvargas@chromium.org

[Sergey Ulanov, 2015-04-02 18:29:15]

+rvargas for src/net OWNERS approval

[rvargas (doing something else), 2015-04-02 18:33:10]

rbstmp lgtm

[Mark Mentovai, 2015-04-02 18:52:36]

LGTM

[Sergey Ulanov, 2015-04-02 18:53:41]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-04-02 18:54:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1053873003/80001

[commit-bot: I haz the power, 2015-04-02 19:46:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-02 19:46:16]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Sergey Ulanov, 2015-04-02 20:48:01]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-04-02 20:48:02]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
rvargas@chromium.org
Link to the patchset: https://codereview.chromium.org/1053873003/#ps100001
(title: "!iOS")

[commit-bot: I haz the power, 2015-04-02 20:50:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1053873003/100001

[Mark Mentovai, 2015-04-02 21:00:57]

https://codereview.chromium.org/1053873003/diff/100001/net/udp/udp_socket_lib...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/1053873003/diff/100001/net/udp/udp_socket_lib...
net/udp/udp_socket_libevent.cc:175: #if defined(OS_MACOSX)
&& !defined(OS_IOS) here and on line 208 too?

[Sergey Ulanov, 2015-04-02 21:01:20]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-04-02 21:01:20]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
rvargas@chromium.org
Link to the patchset: https://codereview.chromium.org/1053873003/#ps120001
(title: "fix ios compilation")

[Sergey Ulanov, 2015-04-02 21:02:35]

On 2015/04/02 21:00:57, Mark Mentovai wrote:
>
https://codereview.chromium.org/1053873003/diff/100001/net/udp/udp_socket_lib...
> File net/udp/udp_socket_libevent.cc (right):
> 
>
https://codereview.chromium.org/1053873003/diff/100001/net/udp/udp_socket_lib...
> net/udp/udp_socket_libevent.cc:175: #if defined(OS_MACOSX)
> && !defined(OS_IOS) here and on line 208 too?

Yes, fixed in patchset 6

[commit-bot: I haz the power, 2015-04-02 21:02:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1053873003/120001

[Mark Mentovai, 2015-04-02 21:04:04]

LGTM

[commit-bot: I haz the power, 2015-04-02 21:16:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-02 21:16:57]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Sergey Ulanov, 2015-04-02 21:56:56]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-04-02 21:56:56]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
rvargas@chromium.org
Link to the patchset: https://codereview.chromium.org/1053873003/#ps130001
(title: " ")

[commit-bot: I haz the power, 2015-04-02 21:59:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1053873003/130001

[commit-bot: I haz the power, 2015-04-03 05:24:45]

Committed patchset #7 (id:130001)

[commit-bot: I haz the power, 2015-04-03 20:32:12]

Patchset 7 (id:??) landed as
https://crrev.com/509aba26b3c3d65cebb1647cb03d8ad1db10eb28
Cr-Commit-Position: refs/heads/master@{#323624}