ensure file descriptors given to pass_fds are inheritable

[jstewmon]

this patch ports a subset of the code that was added to
_posixsubprocess.c and filutils.c to resolve #18571 (Implementation
of the PEP 446: non-inheritable file descriptors) in cpython 3.4

fixes #4

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[jstewmon]

I signed it!

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[googlebot]

CLAs look good, thanks!

[gpshead]

The third parameter to this call is "raise", and we cannot raise an exception in the only context we call make_inheritable() from. It needs to be 0... but...

I suggest simplifying this patch greatly: Cut out the unnecessary bits of set_inheritable() given that the only use of the function is here and should pass raise=0 and atomic_flag_works=NULL which will cut out a lot of unreachable code and get rid of the need for get_inheritable() in _posixsubprocess_helpers.c.

[jstewmon]

Thanks for the feedback.

I see that it makes sense to remove atomic_flag_works and simplify the conditional logic that uses it, since the only call site passes NULL for it.

I don't understand why an exception cannot be raised. I'd like to understand the reason for that. Is it because child_exec expects -1 to be returned for any error case, so that execution jumps to the error label?

[gpshead]

This extension module isn't useful on Windows, get rid of the #ifdef MS_WINDOWS sections to simplify the code further.

[jstewmon]

I saw a couple of recent bumps on #4, so I went ahead and updated the PR as requested by @gpshead, though I still don't know exactly why we must return -1 instead of raising...

[gpshead]

I believe I inadvertently implemented a simpler version of this in b41c3b9 for #4 without remembering that this PR existed. Oops. :)

I didn't bother fixing the pure python implementation in that commit (your two lines added to subprocess32.py) as that code path, while still there and tested as part of the unittest suite, is not the point of the module. The _posixsubprocess32 extension module should always be available.

Other differences: I didn't bother reporting errors at all while trying to set the pass_fds inheritable - it makes a best effort and skips any fd resulting in an fcntl error. I also only implemented the simpler fcntl code path as the complexity of #ifdefs for the other potential API calls did not seem worthwhile for this purpose.

[gpshead]

... anyways, closing this PR given it isn't directly relevant as is anymore. If we find a need improvements to what I already put in, make new PRs for that.