Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes

[Johannes Sixt]

Am 24.10.2016 um 21:53 schrieb Lars Schneider:



On 24 Oct 2016, at 21:22, Johannes Sixt  wrote:

Am 24.10.2016 um 20:03 schrieb larsxschnei...@gmail.com:

From: Lars Schneider 

This fixes "convert: add filter..process option" (edcc8581) on
Windows.


Today's next falls flat on its face on Windows in t0021.15
"required process filter should filter data"; might it be the
failure meant here?(I haven't dug deeper, yet.)


Yes, this is the failure meant here :-)


I trust your word and Dscho's that it fixes a failure on Windows. In my 
case, however, it was an outdated perl (5.8.8) which left a message 
along the lines of "lookup of member flush in IO::Handle failed" in one 
of the *.log files. I came up with the following workaround.


Fix-method: shot-in-the-dark
Perl-foo: not-present
Not-signed-off-by: Me

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index ae4c50f..bb88c5f 100755
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -22,6 +22,7 @@

 use strict;
 use warnings;
+use FileHandle;

 my $MAX_PACKET_CONTENT_SIZE = 65516;
 my @capabilities= @ARGV;
--
2.10.0.343.g37bc62b

Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes

[Junio C Hamano]

Johannes Schindelin  writes:

> That still looks overly complicated, repeatedly ORing cloexec and
> recursing without need. How about this instead?
>
>   static int oflags = O_RDONLY | O_CLOEXEC;
>   int fd = open(ce->name, oflags);
>
>   if ((O_CLOEXEC & oflags) && fd < 0 && errno == EINVAL) {
>   /* Try again w/o O_CLOEXEC: the kernel might not support it */
>   oflags &= ~O_CLOEXEC;
>   fd = open(ce->name, oflags);
>   }

I deliberately separated the part that can and designed to be
toggled (O_CLOEXEC) and the part that is meant to be constant
(O_RDONLY), and I do not think the first part of suggestion is
particularly a good idea.

I didn't write the same open twice, but I agree that an extra "we
fallback to opening with a different flags" inside the if () { }
block that is there exactly for implementing that fallback is an
excellent idea.  I like that part of the suggestion.

Thanks.

Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes

[Johannes Schindelin]

Hi Junio,

On Mon, 24 Oct 2016, Junio C Hamano wrote:

> Eric Wong  writes:
> 
> > larsxschnei...@gmail.com wrote:
> >> +++ b/read-cache.c
> >> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, 
> >> struct stat *st)
> >>  static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
> >>  {
> >>int match = -1;
> >> -  int fd = open(ce->name, O_RDONLY);
> >> +  int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
> >> +
> >> +  if (O_CLOEXEC && fd < 0 && errno == EINVAL)
> >> +  /* Try again w/o O_CLOEXEC: the kernel might not support it */
> >> +  fd = open(ce->name, O_RDONLY);
> >
> > In the case of O_CLOEXEC != 0 and repeated EINVALs,
> > it'd be good to use something like sha1_file_open_flag as in 1/2
> > so we don't repeatedly hit EINVAL.  Thanks.
> 
> Sounds sane.  
> 
> It's just only once, so perhaps we do not mind a recursion like
> this?
> 
>  read-cache.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index b594865d89..a6978b9321 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -156,11 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, 
> struct stat *st)
>  static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
>  {
>   int match = -1;
> - int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
> + static int cloexec = O_CLOEXEC;
> + int fd = open(ce->name, O_RDONLY | cloexec);
>  
> - if (O_CLOEXEC && fd < 0 && errno == EINVAL)
> + if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
>   /* Try again w/o O_CLOEXEC: the kernel might not support it */
> - fd = open(ce->name, O_RDONLY);
> + cloexec &= ~O_CLOEXEC;
> + return ce_compare_data(ce, st);
> + }
>  

That still looks overly complicated, repeatedly ORing cloexec and
recursing without need. How about this instead?

static int oflags = O_RDONLY | O_CLOEXEC;
int fd = open(ce->name, oflags);

if ((O_CLOEXEC & oflags) && fd < 0 && errno == EINVAL) {
/* Try again w/o O_CLOEXEC: the kernel might not support it */
oflags &= ~O_CLOEXEC;
fd = open(ce->name, oflags);
}

Ciao,
Dscho

Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes

[Lars Schneider]

> On 24 Oct 2016, at 21:22, Johannes Sixt  wrote:
> 
> Am 24.10.2016 um 20:03 schrieb larsxschnei...@gmail.com:
>> From: Lars Schneider 
>> 
>> This fixes "convert: add filter..process option" (edcc8581) on
>> Windows.
> 
> Today's next falls flat on its face on Windows in t0021.15 "required process 
> filter should filter data"; might it be the failure meant here? (I haven't 
> dug deeper, yet.)

Yes, this is the failure meant here :-)

Cheers,
Lars

Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes

[Junio C Hamano]

Eric Wong  writes:

> larsxschnei...@gmail.com wrote:
>> +++ b/read-cache.c
>> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, 
>> struct stat *st)
>>  static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
>>  {
>>  int match = -1;
>> -int fd = open(ce->name, O_RDONLY);
>> +int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
>> +
>> +if (O_CLOEXEC && fd < 0 && errno == EINVAL)
>> +/* Try again w/o O_CLOEXEC: the kernel might not support it */
>> +fd = open(ce->name, O_RDONLY);
>
> In the case of O_CLOEXEC != 0 and repeated EINVALs,
> it'd be good to use something like sha1_file_open_flag as in 1/2
> so we don't repeatedly hit EINVAL.  Thanks.

Sounds sane.  

It's just only once, so perhaps we do not mind a recursion like
this?

 read-cache.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index b594865d89..a6978b9321 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,11 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, struct 
stat *st)
 static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
 {
int match = -1;
-   int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
+   static int cloexec = O_CLOEXEC;
+   int fd = open(ce->name, O_RDONLY | cloexec);
 
-   if (O_CLOEXEC && fd < 0 && errno == EINVAL)
+   if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
/* Try again w/o O_CLOEXEC: the kernel might not support it */
-   fd = open(ce->name, O_RDONLY);
+   cloexec &= ~O_CLOEXEC;
+   return ce_compare_data(ce, st);
+   }
 
if (fd >= 0) {
unsigned char sha1[20];

Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes

[Johannes Sixt]

Am 24.10.2016 um 20:03 schrieb larsxschnei...@gmail.com:

From: Lars Schneider 

This fixes "convert: add filter..process option" (edcc8581) on
Windows.


Today's next falls flat on its face on Windows in t0021.15 "required 
process filter should filter data"; might it be the failure meant here? 
(I haven't dug deeper, yet.)


++ test_config_global filter.protocol.process 
'/d/Src/mingw-git/t/t0021/rot13-filter.pl clean smudge'
++ test_when_finished 'test_unconfig --global 
'\''filter.protocol.process'\'''

++ test 0 = 0
++ test_cleanup='{ test_unconfig --global '\''filter.protocol.process'\''
} && (exit "$eval_ret"); eval_ret=$?; :'
++ git config --global filter.protocol.process 
'/d/Src/mingw-git/t/t0021/rot13-filter.pl clean smudge'

++ test_config_global filter.protocol.required true
++ test_when_finished 'test_unconfig --global 
'\''filter.protocol.required'\'''

++ test 0 = 0
++ test_cleanup='{ test_unconfig --global '\''filter.protocol.required'\''
} && (exit "$eval_ret"); eval_ret=$?; { test_unconfig 
--global '\''filter.protocol.process'\''

} && (exit "$eval_ret"); eval_ret=$?; :'
++ git config --global filter.protocol.required true
++ rm -rf repo
++ mkdir repo
++ cd repo
++ git init
Initialized empty Git repository in d:/Src/mingw-git/t/trash 
directory.t0021-conversion/repo/.git/

++ echo git-stderr.log
++ echo '*.r filter=protocol'
++ git add .
++ git commit . -m 'test commit 1'
[master (root-commit) aa5dd37] test commit 1
 Author: A U Thor 
 2 files changed, 2 insertions(+)
 create mode 100644 .gitattributes
 create mode 100644 .gitignore
++ git branch empty-branch
++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test.o' test.r
++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test2.o' test2.r
++ mkdir testsubdir
++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test3 
'\''sq'\'',$x.o' 'testsubdir/test3 '\''sq'\'',$x.r'

+++ file_size test.r
+++ cat test.r
+++ wc -c
+++ sed 's/^[ ]*//'
++ S=57
+++ file_size test2.r
+++ cat test2.r
+++ wc -c
+++ sed 's/^[ ]*//'
++ S2=14
+++ file_size 'testsubdir/test3 '\''sq'\'',$x.r'
+++ cat 'testsubdir/test3 '\''sq'\'',$x.r'
+++ wc -c
+++ sed 's/^[ ]*//'
++ S3=49
++ filter_git add .
++ rm -f rot13-filter.log
++ git add .
error: last command exited with $?=128
not ok 15 - required process filter should filter data

Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes

[Eric Wong]

larsxschnei...@gmail.com wrote:
> +++ b/read-cache.c
> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct 
> stat *st)
>  static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
>  {
>   int match = -1;
> - int fd = open(ce->name, O_RDONLY);
> + int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
> +
> + if (O_CLOEXEC && fd < 0 && errno == EINVAL)
> + /* Try again w/o O_CLOEXEC: the kernel might not support it */
> + fd = open(ce->name, O_RDONLY);

In the case of O_CLOEXEC != 0 and repeated EINVALs,
it'd be good to use something like sha1_file_open_flag as in 1/2
so we don't repeatedly hit EINVAL.  Thanks.

[PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes

[larsxschneider]

From: Lars Schneider 

This fixes "convert: add filter..process option" (edcc8581) on
Windows.

Consider the case of a file that requires filtering and is present in
branch A but not in branch B. If A is the current HEAD and we checkout B
then the following happens:

1. ce_compare_data() opens the file
2.   index_fd() detects that the file requires to run a clean filter and
 calls index_stream_convert_blob()
4. index_stream_convert_blob() calls convert_to_git_filter_fd()
5.   convert_to_git_filter_fd() calls apply_filter() which creates a
 new long running filter process (in case it is the first file
 of this kind to be filtered)
6.   The new filter process inherits all file handles. This is the
 default on Linux/OSX and is explicitly defined in the
 `CreateProcessW` call in `mingw.c` on Windows.
7. ce_compare_data() closes the file
8. Git unlinks the file as it is not present in B

The unlink operation does not work on Windows because the filter process
has still an open handle to the file. On Linux/OSX the unlink operation
succeeds but the file descriptors still leak into the child process.

Fix this problem by opening files in read-cache with the CLOEXEC flag to
ensure that the file descriptor does not remain open in a newly spawned
process similar to 05d1ed61.

Signed-off-by: Lars Schneider 
---
 read-cache.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index 38d67fa..b594865 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct 
stat *st)
 static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
 {
int match = -1;
-   int fd = open(ce->name, O_RDONLY);
+   int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
+
+   if (O_CLOEXEC && fd < 0 && errno == EINVAL)
+   /* Try again w/o O_CLOEXEC: the kernel might not support it */
+   fd = open(ce->name, O_RDONLY);
 
if (fd >= 0) {
unsigned char sha1[20];
-- 
2.10.0