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

2016-09-07 Thread 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

2016-09-07 Thread 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

2016-09-06 Thread 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

2016-09-06 Thread 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

2016-09-05 Thread 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.

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

2016-09-05 Thread larsxschneider 
From: Lars Schneider 

All processes that the Git main process spawns inherit the open file
descriptors of the main process. These leaked file descriptors can
cause problems.

Use the CLOEXEC flag similar to 05d1ed61 to fix the leaked file
descriptors.

Signed-off-by: Lars Schneider 
---
 builtin/pack-objects.c |  2 +-
 cache.h|  2 +-
 pack-bitmap.c  |  2 +-
 sha1_file.c| 14 +++---
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 4a63398..a2b1fb6 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -718,7 +718,7 @@ static off_t write_reused_pack(struct sha1file *f)
if (!is_pack_valid(reuse_packfile))
die("packfile is invalid: %s", reuse_packfile->pack_name);
 
-   fd = git_open_noatime(reuse_packfile->pack_name);
+   fd = git_open_noatime_cloexec(reuse_packfile->pack_name);
if (fd < 0)
die_errno("unable to open packfile for reuse: %s",
  reuse_packfile->pack_name);
diff --git a/cache.h b/cache.h
index b780a91..ae79747 100644
--- a/cache.h
+++ b/cache.h
@@ -1089,7 +1089,7 @@ extern int write_sha1_file(const void *buf, unsigned long 
len, const char *type,
 extern int hash_sha1_file_literally(const void *buf, unsigned long len, const 
char *type, unsigned char *sha1, unsigned flags);
 extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned 
char *);
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);
-extern int git_open_noatime(const char *name);
+extern int git_open_noatime_cloexec(const char *name);
 extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
 extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, 
unsigned long mapsize, void *buffer, unsigned long bufsiz);
 extern int parse_sha1_header(const char *hdr, unsigned long *sizep);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index b949e51..1b39e5d 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -266,7 +266,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)
return -1;
 
idx_name = pack_bitmap_filename(packfile);
-   fd = git_open_noatime(idx_name);
+   fd = git_open_noatime_cloexec(idx_name);
free(idx_name);
 
if (fd < 0)
diff --git a/sha1_file.c b/sha1_file.c
index 3045aea..c1701dc 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -356,7 +356,7 @@ void read_info_alternates(const char * relative_base, int 
depth)
int fd;
 
path = xstrfmt("%s/info/alternates", relative_base);
-   fd = git_open_noatime(path);
+   fd = git_open_noatime_cloexec(path);
free(path);
if (fd < 0)
return;
@@ -550,7 +550,7 @@ static int check_packed_git_idx(const char *path, struct 
packed_git *p)
struct pack_idx_header *hdr;
size_t idx_size;
uint32_t version, nr, i, *index;
-   int fd = git_open_noatime(path);
+   int fd = git_open_noatime_cloexec(path);
struct stat st;
 
if (fd < 0)
@@ -956,7 +956,7 @@ static int open_packed_git_1(struct packed_git *p)
while (pack_max_fds <= pack_open_fds && close_one_pack())
; /* nothing */
 
-   p->pack_fd = git_open_noatime(p->pack_name);
+   p->pack_fd = git_open_noatime_cloexec(p->pack_name);
if (p->pack_fd < 0 || fstat(p->pack_fd, ))
return -1;
pack_open_fds++;
@@ -1459,9 +1459,9 @@ int check_sha1_signature(const unsigned char *sha1, void 
*map,
return hashcmp(sha1, real_sha1) ? -1 : 0;
 }
 
-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;
@@ -1505,7 +1505,7 @@ static int open_sha1_file(const unsigned char *sha1)
struct alternate_object_database *alt;
int most_interesting_errno;
 
-   fd = git_open_noatime(sha1_file_name(sha1));
+   fd = git_open_noatime_cloexec(sha1_file_name(sha1));
if (fd >= 0)
return fd;
most_interesting_errno = errno;
@@ -1513,7 +1513,7 @@ static int open_sha1_file(const unsigned char *sha1)
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt->next) {
fill_sha1_path(alt->name, sha1);
-   fd = git_open_noatime(alt->base);
+   fd = git_open_noatime_cloexec(alt->base);
if (fd >= 0)
return fd;
if (most_interesting_errno == ENOENT)
-- 
2.10.0