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

[Eric Wong]

Lars Schneider  wrote:
> > On 06 Sep 2016, at 13:38, Johannes Schindelin  
> > wrote:
> > On Mon, 5 Sep 2016, Eric Wong wrote:
> >> larsxschnei...@gmail.com wrote:
> >>> -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()" ?

Maybe "_noatime" is useful in some cases, but maybe not *shrug*

My original point for removing the "_cloexec" suffix was that
(at least for Perl and Ruby), cloexec-by-default was so prevalent
in FD-creating syscalls that having the suffix wasn't needed.

> >> 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.


> 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)) {

80 columns overflow

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

But otherwise much better since it doesn't blindly zero
sha1_file_open_flag :>

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

[Lars Schneider]

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


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

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

[Johannes Schindelin]

Hi Eric & Lars,

On Mon, 5 Sep 2016, Eric Wong wrote:

> larsxschnei...@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;
> 
> If there's real problems being caused by lack of cloexec
> today, I think the F_SETFD fallback I proposed in
> https://public-inbox.org/git/20160818173555.GA29253@starla/
> will be necessary.

Yes, it is good to have that patch available to go if we need it. I do not
think that we will need it, though, as the biggest problems that are
solved through the CLOEXEC flag are ones caused on Windows, when files
cannot be deleted or renamed because there are still (uselessly) open
handles referencing them.

> 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.

> 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.

Ciao,
Dscho

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

[Jakub Narębski]

W dniu 06.09.2016 o 00:27, Eric Wong pisze:
> larsxschnei...@gmail.com wrote:

>> -int git_open_noatime(const char *name)
>> +int git_open_noatime_cloexec(const char *name)
[...]
> 
> I question the need for the "_cloexec" suffixing in the
> function name since the old function is going away entirely.

On the other hand the new name is descriptive...

> 
> 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.
> 
> 
> Fwiw, Perl has been setting cloexec on FDs above $^F
> (2, $SYSTEM_FD_MAX) for decades, and Ruby started
> doing it a few years ago, too.
 

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

[Eric Wong]

larsxschnei...@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;

If there's real problems being caused by lack of cloexec
today, I think the F_SETFD fallback I proposed in
https://public-inbox.org/git/20160818173555.GA29253@starla/
will be necessary.

I question the need for the "_cloexec" suffixing in the
function name since the old function is going away entirely.

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.


Fwiw, Perl has been setting cloexec on FDs above $^F
(2, $SYSTEM_FD_MAX) for decades, and Ruby started
doing it a few years ago, too.