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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> On 06 Sep 2016, at 13:38, Johannes Schindelin <johannes.schindelin@xxxxxx> wrote:
> 
> Hi Eric & Lars,
> 
> On Mon, 5 Sep 2016, Eric Wong wrote:
> 
>> larsxschneider@xxxxxxxxx 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()" ?


>> 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;
+               }





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]