1278361 - IPC code shouldn't wrap close() with Chromium HANDLE_EINTR

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

[Jed Davis [:jld] (on leave until 08-31)]

I notice we have code, both in ipc/chromium/src and ipc/glue, doing HANDLE_EINTR(close(...)).  In theory this is POSIX conformant; in practice it risks doing a double-close (and maybe closing some other thread's fd), and the situation is irritatingly complicated.

https://crbug.com/269623 is the upstream bug; it goes into the details, and how they got close() to behave in a manageable way both on OS X and on Linux with glibc. 

https://ewontfix.com/4/ is another discussion of the underlying issue, focusing on Linux.

Comment 1

[Andrew Overholt [:overholt]]

What should we do here?

Comment 2

[Jed Davis [:jld] (on leave until 08-31)]

Chromium's solution was to use IGNORE_EINTR (which we don't have, but it's not terribly hard to write or import) — and, on platforms where it's possible for that to leak the fd due to pthread cancellation support, to shim close() to call a lower-level implementation that doesn't have that feature/flaw.  Note that the first part by itself will still prevent use-after-close, and that the second part (avoiding fd leaks) would also apply to non-IPC code that calls close() and typically doesn't check for errors.

Comment 3

[Jed Davis [:jld] (on leave until 08-31)]

So I just rediscovered this.  I don't know what I was thinking when I wrote comment #0; this is not POSIX conformant (and that's not the right adjective anyway) because POSIX allows the behavior of Linux and *BSD and Solaris and AIX, where it's a double-close.  It *is* the correct approach on HP-UX, but we don't appear to have supported HP-UX since approximately Firefox 3.

Chromium dealt with this a while ago; see https://crbug.com/269623, and it looks like they now have a pre-commit hook that rejects this pattern.  I'm not sure if we have to care about the cases involving pthread cancellation, because I don't think we use that (and wouldn't it just terminate the thread without giving it a chance to retry?).

Also, this may be related to the intermittent failures in bug 1243108; I didn't think a simple double-close bug could explain what we were seeing there but I could be wrong about that.

Comment 4

[Jed Davis [:jld] (on leave until 08-31)]

I have patches.

Comment 5

[Jed Davis [:jld] (on leave until 08-31)]

Attachment: Bug 1278361 - Step 1: Update eintr_wrapper.h to bring in IGNORE_EINTR.

This is based on the current security/sandbox/chromium version of eintr_wrapper.h,
taken from upstream commit 937db09514e061d7983e90e0c448cfa61680f605.

I've edited it to remove some things that aren't relevant to us: the
debug-mode loop limit in HANDLE_EINTR, because we don't seem to be
having the problem it's meant to fix and it risks regressions, and
references to Fuchsia, which we don't (yet) support.  I also kept the
original include guards (the file path has changed upstream).

What this patch *does* do is add IGNORE_EINTR and modernize the C++
slightly (using decltype instead of nonstandard typeof).

Review commit: https://reviewboard.mozilla.org/r/235856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235856/

Comment 6

[Jed Davis [:jld] (on leave until 08-31)]

Attachment: Bug 1278361 - Step 2: Search-and-replace HANDLE_EINTR(close(...)) to use IGNORE_EINTR.

I've also manually verified that no other references to HANDLE_EINTR are
wrapping a close() in any less syntactically obvious way.

Review commit: https://reviewboard.mozilla.org/r/235858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235858/

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8967197 [details]
Bug 1278361 - Step 1: Update eintr_wrapper.h to bring in IGNORE_EINTR.

https://reviewboard.mozilla.org/r/235856/#review241790

Thank you for wading in to update some of our ipc import.

::: ipc/chromium/src/base/eintr_wrapper.h:14
(Diff revision 1)
>  //
> -// On Windows, this wrapper macro does nothing.
> +// On Windows, this wrapper macro does nothing because there are no
> +// signals.
> +//
> +// Don't wrap close calls in HANDLE_EINTR. Use IGNORE_EINTR if the return
> +// value of close is significant. See http://crbug.com/269623.

Reading this issue and some of the linked bits is all sorts of sadness.  (I can't understand POSIX's stance that "because HP-UX does this, we can't change the POSIX behavior"...)

Do we want to try importing something like https://chromium.googlesource.com/chromium/src/+/36424441697b957009738f609c0eae22cf5ded34%5E%21/ for our Mac builds?

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8967198 [details]
Bug 1278361 - Step 2: Search-and-replace HANDLE_EINTR(close(...)) to use IGNORE_EINTR.

https://reviewboard.mozilla.org/r/235858/#review241792

It would be excellent if we had the `PRESUBMIT` hook discussed in the Chromium issue to enforce this behavior, but ideally people will do the right thing by cargo-culting.

Comment 9

[Jed Davis [:jld] (on leave until 08-31)]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> Do we want to try importing something like
> https://chromium.googlesource.com/chromium/src/+/
> 36424441697b957009738f609c0eae22cf5ded34%5E%21/ for our Mac builds?

I don't think we need that, because we're not using pthread cancellation.  (And I don't think we'd ever want to — abruptly terminating a thread at a semi-unpredictable point seems like it'd have the same kind of problem as exception safety, but more so.)

Comment 10

[Jed Davis [:jld] (on leave until 08-31)]

The other thing about cancelling close() is: as I speculated in comment #3, it won't actually return EINTR; the thread is canceled and close() never returns at all.

The kernel *does* return EINTR to userspace — as noted on the Chromium bug — but for syscalls that are cancellation points, the error case in the libsystem wrapper function, which normally just sets errno, also checks for cancellation via the descriptively named _pthread_exit_if_canceled.

(In addition to source-diving, I've also verified this with a small test program.)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/062265dbddb2
Step 1: Update eintr_wrapper.h to bring in IGNORE_EINTR. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/48d171934825
Step 2: Search-and-replace HANDLE_EINTR(close(...)) to use IGNORE_EINTR. r=froydnj

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/062265dbddb2
https://hg.mozilla.org/mozilla-central/rev/48d171934825