Re: [PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC

From: Lars Schneider <larsxschneider@gmail.com>
Date: Sep 7, 2016, 13:20 UTC
Message-ID: <F8E7B7CE-1177-4CBD-999E-21C593A8ACD2@gmail.com>
In-Reply-To: <alpine.DEB.2.20.1609061333030.129229@virtualbox>

Show 26 quoted lines

> On 06 Sep 2016, at 13:38, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> 
> Hi Eric & Lars,
> 
> On Mon, 5 Sep 2016, Eric Wong wrote:
> 
>> larsxschneider@gmail.com wrote:
>>> All processes that the Git main process spawns inherit the open file
>>> descriptors of the main process. These leaked file descriptors can
>>> cause problems.
>> 
>> 
>>> -int git_open_noatime(const char *name)
>>> +int git_open_noatime_cloexec(const char *name)
>>> {
>>> -	static int sha1_file_open_flag = O_NOATIME;
>>> +	static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
>>> 
>>> 	for (;;) {
>>> 		int fd;
> 
>> I question the need for the "_cloexec" suffixing in the
>> function name since the old function is going away entirely.
> 
> Me, too. While it is correct, it makes things harder to read, so it may
> even cause more harm than it does good.

What name would you suggest? Leaving the name as-is seems misleading to me. Maybe just "git_open()" ?

Show 14 quoted lines

>> I prefer all FD-creating functions set cloexec by default
>> for FD > 2 to avoid inadvertantly leaking FDs.  So we
>> ought to use pipe2, accept4, socket(..., SOCK_CLOEXEC), etc...
>> and fallback to the racy+slower F_SETFD when not available.
> 
> In the original Pull Request where the change was contributed to Git for
> Windows, this was tested (actually, the code did not see whether fd > 2,
> but simply assumed that all newly opened file descriptors would be > 2
> anyway), and it failed:
> 
> https://github.com/git-for-windows/git/pull/755#issuecomment-220247972
> 
> So it appears that we would have to exclude at least the code path to `git
> upload-pack` from that magic.

I just realized that Dscho improved his original patch in GfW with a fallback if CLOEXEC is not present.

I applied the same mechanism here. Would that be OK?

Thanks, Lars

-       static int sha1_file_open_flag = O_NOATIME;
+       static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;

        for (;;) {
                int fd;

@@ -1471,12 +1471,17 @@ int git_open_noatime(const char *name)
                if (fd >= 0)
                        return fd;

-               /* Might the failure be due to O_NOATIME? */
-               if (errno != ENOENT && sha1_file_open_flag) {
-                       sha1_file_open_flag = 0;
+               /* Try again w/o O_CLOEXEC: the kernel might not support it */
+               if (O_CLOEXEC && errno == EINVAL && (sha1_file_open_flag & O_CLOEXEC)) {
+                       sha1_file_open_flag &= ~O_CLOEXEC;
                        continue;
                }

+               /* Might the failure be due to O_NOATIME? */
+               if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
+                       sha1_file_open_flag &= ~O_NOATIME;
+                       continue;
+               }