[PATCH 0/2] open() error checking

[PATCH 0/2] open() error checking

[Thomas Rast]

#1 is Dale's suggested change.  Dale, to include it we'd need your
Signed-off-by as per Documentation/SubmittingPatches.

#2 is a similar error-checking fix; I reviewed 'git grep "\bopen\b"'
and found one case where the return value was obviously not tested.
The corresponding Windows code path has the same problem, but I dare
not touch it; perhaps someone from the Windows side can look into it?

I originally had a four-patch series to open 0/1/2 from /dev/null, but
then I noticed that this was shot down in 2008:

  http://thread.gmane.org/gmane.comp.version-control.git/93605/focus=93896

Do you want to resurrect this?

The worst part about it is that because we don't have a stderr to rely
on, we can't simply die("stop playing mind games").


Dale R. Worley (1):
  git_mkstemps: correctly test return value of open()

Thomas Rast (1):
  run-command: dup_devnull(): guard against syscalls failing

 run-command.c | 5 ++++-
 wrapper.c     | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
1.8.3.2.998.g1d087bc

[PATCH 1/2] git_mkstemps: correctly test return value of open()

[Thomas Rast]

From: "Dale R. Worley" <worley@alum.mit.edu>

open() returns -1 on failure, and indeed 0 is a possible success value
if the user closed stdin in our process.  Fix the test.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 wrapper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/wrapper.c b/wrapper.c
index dd7ecbb..6a015de 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 		template[5] = letters[v % num_letters]; v /= num_letters;
 
 		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
-		if (fd > 0)
+		if (fd >= 0)
 			return fd;
 		/*
 		 * Fatal error (EPERM, ENOSPC etc).
-- 
1.8.3.2.998.g1d087bc

[PATCH 2/2] run-command: dup_devnull(): guard against syscalls failing

[Thomas Rast]

dup_devnull() did not check the return values of open() and dup2().
Fix this omission.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 run-command.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index aece872..1b7f88e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -76,7 +76,10 @@ static inline void close_pair(int fd[2])
 static inline void dup_devnull(int to)
 {
 	int fd = open("/dev/null", O_RDWR);
-	dup2(fd, to);
+	if (fd < 0)
+		die_errno(_("open /dev/null failed"));
+	if (dup2(fd, to) < 0)
+		die_errno(_("dup2(%d,%d) failed"), fd, to);
 	close(fd);
 }
 #endif
-- 
1.8.3.2.998.g1d087bc

Re: [PATCH 0/2] open() error checking

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> #1 is Dale's suggested change.  Dale, to include it we'd need your
> Signed-off-by as per Documentation/SubmittingPatches.
>
> #2 is a similar error-checking fix; I reviewed 'git grep "\bopen\b"'
> and found one case where the return value was obviously not tested.
> The corresponding Windows code path has the same problem, but I dare
> not touch it; perhaps someone from the Windows side can look into it?
>
> I originally had a four-patch series to open 0/1/2 from /dev/null, but
> then I noticed that this was shot down in 2008:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/93605/focus=93896

The way I recall the thread was not "shot down" but more like
"fizzled out without seeing a clear consensus".  As a normal POSIX
program, we do rely on fd#2 connected to an error stream, and I do
agree with the general sentiment of that old thread that it is very
wrong for warning() or die() to write to a pipe or file descriptor
we opened for some other purpose, corrupting the destination.

I briefly wondered if we can do the sanity check lazily (e.g. upon
first warning() see of fd#2 is open and otherwise die silently), but
we may open a fd (e.g. to create a new loose object) that may happen
to grab fd#2 and then it is too late for us to do anything about it,
so...

> Do you want to resurrect this?
>
> The worst part about it is that because we don't have a stderr to rely
> on, we can't simply die("stop playing mind games").

Right.

Re: [PATCH 0/2] open() error checking

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> I originally had a four-patch series to open 0/1/2 from /dev/null, but
>> then I noticed that this was shot down in 2008:
>>
>>   http://thread.gmane.org/gmane.comp.version-control.git/93605/focus=93896
>
> The way I recall the thread was not "shot down" but more like
> "fizzled out without seeing a clear consensus".  As a normal POSIX
> program, we do rely on fd#2 connected to an error stream, and I do
> agree with the general sentiment of that old thread that it is very
> wrong for warning() or die() to write to a pipe or file descriptor
> we opened for some other purpose, corrupting the destination.
>
> I briefly wondered if we can do the sanity check lazily (e.g. upon
> first warning() see of fd#2 is open and otherwise die silently), but
> we may open a fd (e.g. to create a new loose object) that may happen
> to grab fd#2 and then it is too late for us to do anything about it,
> so...

I think we'd have to do it on startup.  Since we do many things already,
a few extra dup calls should hardly matter.

I'll send the patches in reply in a minute, I had them lying around
already.  But if you (again) decide that it's not worth it, I don't care
too deeply.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[PATCH 1/2] daemon/shell: refactor redirection of 0/1/2 from /dev/null

[Thomas Rast]

Both daemon.c and shell.c contain logic to open FDs 0/1/2 from
/dev/null if they are not already open.  Move the function in daemon.c
to setup.c and use it in shell.c, too.

While there, remove a 'not' that inverted the meaning of the comment.
The point is indeed to *avoid* messing up.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 cache.h  |  2 ++
 daemon.c | 12 ------------
 setup.c  | 12 ++++++++++++
 shell.c  | 12 +++---------
 4 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/cache.h b/cache.h
index dd0fb33..f007724 100644
--- a/cache.h
+++ b/cache.h
@@ -425,6 +425,8 @@ extern void verify_filename(const char *prefix,
 extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, int);
 extern int init_db(const char *template_dir, unsigned int flags);
 
+extern void sanitize_stdfds(void);
+
 #define alloc_nr(x) (((x)+16)*3/2)
 
 /*
diff --git a/daemon.c b/daemon.c
index 6aeddcb..973ec38 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1047,18 +1047,6 @@ static int service_loop(struct socketlist *socklist)
 	}
 }
 
-/* if any standard file descriptor is missing open it to /dev/null */
-static void sanitize_stdfds(void)
-{
-	int fd = open("/dev/null", O_RDWR, 0);
-	while (fd != -1 && fd < 2)
-		fd = dup(fd);
-	if (fd == -1)
-		die_errno("open /dev/null or dup failed");
-	if (fd > 2)
-		close(fd);
-}
-
 #ifdef NO_POSIX_GOODIES
 
 struct credentials;
diff --git a/setup.c b/setup.c
index 94c1e61..88aab94 100644
--- a/setup.c
+++ b/setup.c
@@ -908,3 +908,15 @@ const char *resolve_gitdir(const char *suspect)
 		return suspect;
 	return read_gitfile(suspect);
 }
+
+/* if any standard file descriptor is missing open it to /dev/null */
+void sanitize_stdfds(void)
+{
+	int fd = open("/dev/null", O_RDWR, 0);
+	while (fd != -1 && fd < 2)
+		fd = dup(fd);
+	if (fd == -1)
+		die_errno("open /dev/null or dup failed");
+	if (fd > 2)
+		close(fd);
+}
diff --git a/shell.c b/shell.c
index 1429870..66350b2 100644
--- a/shell.c
+++ b/shell.c
@@ -147,7 +147,6 @@ int main(int argc, char **argv)
 	char *prog;
 	const char **user_argv;
 	struct commands *cmd;
-	int devnull_fd;
 	int count;
 
 	git_setup_gettext();
@@ -156,15 +155,10 @@ int main(int argc, char **argv)
 
 	/*
 	 * Always open file descriptors 0/1/2 to avoid clobbering files
-	 * in die().  It also avoids not messing up when the pipes are
-	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
+	 * in die().  It also avoids messing up when the pipes are dup'ed
+	 * onto stdin/stdout/stderr in the child processes we spawn.
 	 */
-	devnull_fd = open("/dev/null", O_RDWR);
-	while (devnull_fd >= 0 && devnull_fd <= 2)
-		devnull_fd = dup(devnull_fd);
-	if (devnull_fd == -1)
-		die_errno("opening /dev/null failed");
-	close (devnull_fd);
+	sanitize_stdfds();
 
 	/*
 	 * Special hack to pretend to be a CVS server
-- 
1.8.3.2.998.g1d087bc

[PATCH 2/2] git: ensure 0/1/2 are open in main()

[Thomas Rast]

Not having an open FD in the 0--2 range can lead to strange results,
for example, a subsequent open() may return 2 (stderr) and then a
die() would clobber this file.

git-daemon and git-shell already guarded against this, but apparently
users also manage to trip over it in other git commands.  So we call
sanitize_stdfds() during main git startup.

Since these FDs are inherited, this covers all use of 'git foo ...',
and all internal C commands when called directly.  It does not fix
shell/perl commands called directly.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 git.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/git.c b/git.c
index 4359086..6104d5e 100644
--- a/git.c
+++ b/git.c
@@ -525,6 +525,13 @@ int main(int argc, char **av)
 	if (!cmd)
 		cmd = "git-help";
 
+	/*
+	 * Always open file descriptors 0/1/2 to avoid clobbering files
+	 * in die().  It also avoids messing up when the pipes are dup'ed
+	 * onto stdin/stdout/stderr in the child processes we spawn.
+	 */
+	sanitize_stdfds();
+
 	git_setup_gettext();
 
 	/*
-- 
1.8.3.2.998.g1d087bc

Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()

[Thomas Rast]

Thomas Rast <trast@inf.ethz.ch> writes:

> From: "Dale R. Worley" <worley@alum.mit.edu>
>
> open() returns -1 on failure, and indeed 0 is a possible success value
> if the user closed stdin in our process.  Fix the test.
>
> Signed-off-by: Thomas Rast <trast@inf.ethz.ch>

I see you have this in 'pu' without Dale's signoff.  I'm guessing
(IANAL) that it's too small to be copyrighted and anyway there is only
way to fix it, but maybe Dale can "sign off" just to be safe, anyway?

>  wrapper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/wrapper.c b/wrapper.c
> index dd7ecbb..6a015de 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  		template[5] = letters[v % num_letters]; v /= num_letters;
>  
>  		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
> -		if (fd > 0)
> +		if (fd >= 0)
>  			return fd;
>  		/*
>  		 * Fatal error (EPERM, ENOSPC etc).

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> From: "Dale R. Worley" <worley@alum.mit.edu>
>>
>> open() returns -1 on failure, and indeed 0 is a possible success value
>> if the user closed stdin in our process.  Fix the test.
>>
>> Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
>
> I see you have this in 'pu' without Dale's signoff.  I'm guessing
> (IANAL) that it's too small to be copyrighted and anyway there is only
> way to fix it, but maybe Dale can "sign off" just to be safe, anyway?

Yup, that is a good idea.  Thanks.

>
>>  wrapper.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/wrapper.c b/wrapper.c
>> index dd7ecbb..6a015de 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>>  		template[5] = letters[v % num_letters]; v /= num_letters;
>>  
>>  		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
>> -		if (fd > 0)
>> +		if (fd >= 0)
>>  			return fd;
>>  		/*
>>  		 * Fatal error (EPERM, ENOSPC etc).

Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()

[Drew Northup]

I presume that I should apply this change to my porting of 
git_mkstemps_mode() to tig. If there are no complaints about this for a 
couple of days I will do so.

REF: $gmane/229961

On 07/17/2013 03:29 PM, Junio C Hamano wrote:
> Thomas Rast<trast@inf.ethz.ch>  writes:
>> Thomas Rast<trast@inf.ethz.ch>  writes:
>>> From: "Dale R. Worley"<worley@alum.mit.edu>
>>>
>>> open() returns -1 on failure, and indeed 0 is a possible success value
>>> if the user closed stdin in our process.  Fix the test.
>>>
>>> Signed-off-by: Thomas Rast<trast@inf.ethz.ch>

>>>   wrapper.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/wrapper.c b/wrapper.c
>>> index dd7ecbb..6a015de 100644
>>> --- a/wrapper.c
>>> +++ b/wrapper.c
>>> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>>>   		template[5] = letters[v % num_letters]; v /= num_letters;
>>>
>>>   		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
>>> -		if (fd>  0)
>>> +		if (fd>= 0)
>>>   			return fd;
>>>   		/*
>>>   		 * Fatal error (EPERM, ENOSPC etc).


--
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()

[Junio C Hamano]

Drew Northup <n1xim.email@gmail.com> writes:

> I presume that I should apply this change to my porting of
> git_mkstemps_mode() to tig. If there are no complaints about this for
> a couple of days I will do so.

Hmph, Thomas and I were actually asking you to give us

	Signed-off-by: Drew Northup <n1xim.email@gmail.com>

for the patch in question.  If tig has the same issue, applying that
same patch there may make sense, but that is an independent issue.

Thanks.

>
> REF: $gmane/229961
>
> On 07/17/2013 03:29 PM, Junio C Hamano wrote:
>> Thomas Rast<trast@inf.ethz.ch>  writes:
>>> Thomas Rast<trast@inf.ethz.ch>  writes:
>>>> From: "Dale R. Worley"<worley@alum.mit.edu>
>>>>
>>>> open() returns -1 on failure, and indeed 0 is a possible success value
>>>> if the user closed stdin in our process.  Fix the test.
>>>>
>>>> Signed-off-by: Thomas Rast<trast@inf.ethz.ch>
>
>>>>   wrapper.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/wrapper.c b/wrapper.c
>>>> index dd7ecbb..6a015de 100644
>>>> --- a/wrapper.c
>>>> +++ b/wrapper.c
>>>> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>>>>   		template[5] = letters[v % num_letters]; v /= num_letters;
>>>>
>>>>   		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
>>>> -		if (fd>  0)
>>>> +		if (fd>= 0)
>>>>   			return fd;
>>>>   		/*
>>>>   		 * Fatal error (EPERM, ENOSPC etc).
>
>
> --
> -Drew Northup
> --------------------------------------------------------------
> "As opposed to vegetable or mineral error?"
> -John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Drew Northup <n1xim.email@gmail.com> writes:
>
>> I presume that I should apply this change to my porting of
>> git_mkstemps_mode() to tig. If there are no complaints about this for
>> a couple of days I will do so.
>
> Hmph, Thomas and I were actually asking you to give us
>
> 	Signed-off-by: Drew Northup <n1xim.email@gmail.com>

Gaahhh, I need a bit more caffeine.  Somehow I mixed up Dale and
Drew.

Sorry for the noise.  Please ignore.


> for the patch in question.  If tig has the same issue, applying that
> same patch there may make sense, but that is an independent issue.
>
> Thanks.
>
>>
>> REF: $gmane/229961
>>
>> On 07/17/2013 03:29 PM, Junio C Hamano wrote:
>>> Thomas Rast<trast@inf.ethz.ch>  writes:
>>>> Thomas Rast<trast@inf.ethz.ch>  writes:
>>>>> From: "Dale R. Worley"<worley@alum.mit.edu>
>>>>>
>>>>> open() returns -1 on failure, and indeed 0 is a possible success value
>>>>> if the user closed stdin in our process.  Fix the test.
>>>>>
>>>>> Signed-off-by: Thomas Rast<trast@inf.ethz.ch>
>>
>>>>>   wrapper.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/wrapper.c b/wrapper.c
>>>>> index dd7ecbb..6a015de 100644
>>>>> --- a/wrapper.c
>>>>> +++ b/wrapper.c
>>>>> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>>>>>   		template[5] = letters[v % num_letters]; v /= num_letters;
>>>>>
>>>>>   		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
>>>>> -		if (fd>  0)
>>>>> +		if (fd>= 0)
>>>>>   			return fd;
>>>>>   		/*
>>>>>   		 * Fatal error (EPERM, ENOSPC etc).
>>
>>
>> --
>> -Drew Northup
>> --------------------------------------------------------------
>> "As opposed to vegetable or mineral error?"
>> -John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()

[Dale R. Worley]

I've been looking into writing a proper test for this patch.  My first
attempt tests the symptom that was seen initially, that "git commit"
fails if fd 0 is closed.

One problem is how to arrange for fd 0 to be closed.  I could use the
bash redirection "<&-", but I think you want to be more portable than
that.  This version uses execvp() inside a small C program, and
execvp() is a Posix function.

I've tested that this test does what it should:  If you remove the
fix, "fd >= 0", the test fails.  If you then remove "test-close-fd-0"
from before "git init" in the test, the test is nullified and succeeds
again.

Here is the diff.  What do people think of it?

diff --git a/Makefile b/Makefile
index 0600eb4..6b410f5 100644
--- a/Makefile
+++ b/Makefile
@@ -557,6 +557,7 @@ X =
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
 TEST_PROGRAMS_NEED_X += test-chmtime
+TEST_PROGRAMS_NEED_X += test-close-fd-0
 TEST_PROGRAMS_NEED_X += test-ctype
 TEST_PROGRAMS_NEED_X += test-date
 TEST_PROGRAMS_NEED_X += test-delta
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 986b2a8..6a31103 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -25,6 +25,13 @@ test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints file
 	grep "cannotwrite/test" err
 '
 
+test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' '
+	git init &&
+	echo Test. >test-file &&
+	git add test-file &&
+	test-close-fd-0 git commit -m Message.
+'
+
 test_expect_success 'check for a bug in the regex routines' '
 	# if this test fails, re-build git with NO_REGEX=1
 	test-regex
diff --git a/test-close-fd-0.c b/test-close-fd-0.c
new file mode 100644
index 0000000..3745c34
--- /dev/null
+++ b/test-close-fd-0.c
@@ -0,0 +1,14 @@
+#include <unistd.h>
+
+/* Close file descriptor 0 (which is standard-input), then execute the
+ * remainder of the command line as a command. */
+
+int main(int argc, char **argv)
+{
+	/* Close fd 0. */
+	close(0);
+	/* Execute the requested command. */
+	execvp(argv[1], &argv[1]);
+	/* If execve() failed, return an error. */
+	return 1;
+}
diff --git a/wrapper.c b/wrapper.c
index dd7ecbb..6a015de 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 		template[5] = letters[v % num_letters]; v /= num_letters;
 
 		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
-		if (fd > 0)
+		if (fd >= 0)
 			return fd;
 		/*
 		 * Fatal error (EPERM, ENOSPC etc).

Dale

Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()

[Eric Sunshine]

On Thu, Jul 18, 2013 at 4:32 PM, Dale R. Worley <worley@alum.mit.edu> wrote:
> I've been looking into writing a proper test for this patch.  My first
> attempt tests the symptom that was seen initially, that "git commit"
> fails if fd 0 is closed.
>
> One problem is how to arrange for fd 0 to be closed.  I could use the
> bash redirection "<&-", but I think you want to be more portable than
> that.  This version uses execvp() inside a small C program, and
> execvp() is a Posix function.

"<&-" is POSIX.

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_05

> I've tested that this test does what it should:  If you remove the
> fix, "fd >= 0", the test fails.  If you then remove "test-close-fd-0"
> from before "git init" in the test, the test is nullified and succeeds
> again.
>
> Here is the diff.  What do people think of it?

Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()

[Junio C Hamano]

On Thu, Jul 18, 2013 at 1:32 PM, Dale R. Worley <worley@alum.mit.edu> wrote:
> I've been looking into writing a proper test for this patch.  My first
> attempt tests the symptom that was seen initially, that "git commit"
> fails if fd 0 is closed.
>
> One problem is how to arrange for fd 0 to be closed.  I could use the
> bash redirection "<&-", but I think you want to be more portable than
> that.

That's just a plain-vanilla part of POSIX shell behaviour, no?

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_05

Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()

[Dale R. Worley]

> From: Junio C Hamano <gitster@pobox.com>
> 
> That's just a plain-vanilla part of POSIX shell behaviour, no?
> 
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_05

"Close standard input" is so weird I never thought it was Posix.  In
that case, we can eliminate the C helper program:

diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 986b2a8..d427f3a 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -25,6 +25,13 @@ test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints file
 	grep "cannotwrite/test" err
 '
 
+test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' '
+	git init &&
+	echo Test. >test-file &&
+	git add test-file &&
+	git commit -m Message. <&-
+'
+
 test_expect_success 'check for a bug in the regex routines' '
 	# if this test fails, re-build git with NO_REGEX=1
 	test-regex
diff --git a/wrapper.c b/wrapper.c
index dd7ecbb..6a015de 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 		template[5] = letters[v % num_letters]; v /= num_letters;
 
 		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
-		if (fd > 0)
+		if (fd >= 0)
 			return fd;
 		/*
 		 * Fatal error (EPERM, ENOSPC etc).

Is this a sensible place to put this test?

Dale

Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()

[Junio C Hamano]

worley@alum.mit.edu (Dale R. Worley) writes:

>> From: Junio C Hamano <gitster@pobox.com>
>> 
>> That's just a plain-vanilla part of POSIX shell behaviour, no?
>> 
>> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_05
>
> "Close standard input" is so weird I never thought it was Posix.  In
> that case, we can eliminate the C helper program:
>
> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
> index 986b2a8..d427f3a 100755
> --- a/t/t0070-fundamental.sh
> +++ b/t/t0070-fundamental.sh
> @@ -25,6 +25,13 @@ test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints file
>  	grep "cannotwrite/test" err
>  '
>  
> +test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' '
> +	git init &&
> +	echo Test. >test-file &&
> +	git add test-file &&
> +	git commit -m Message. <&-
> +'
> +

Yup.  I wonder how it would fail without the fix, though ;-)

>  test_expect_success 'check for a bug in the regex routines' '
>  	# if this test fails, re-build git with NO_REGEX=1
>  	test-regex
> diff --git a/wrapper.c b/wrapper.c
> index dd7ecbb..6a015de 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  		template[5] = letters[v % num_letters]; v /= num_letters;
>  
>  		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
> -		if (fd > 0)
> +		if (fd >= 0)
>  			return fd;
>  		/*
>  		 * Fatal error (EPERM, ENOSPC etc).
>
> Is this a sensible place to put this test?
>
> Dale

Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()

[Dale R. Worley]

> From: Junio C Hamano <gitster@pobox.com>

> > +test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' '
> > +	git init &&
> > +	echo Test. >test-file &&
> > +	git add test-file &&
> > +	git commit -m Message. <&-
> > +'
> > +
> 
> Yup.  I wonder how it would fail without the fix, though ;-)

Eh, what?  You could run it and see.  The test script system will just
say "not ok" for this test.  If you execute those commands from a
shell, you see:

 $ git init
Initialized empty Git repository in /common/not-replicated/worley/temp/1/.git/
 $ echo Test. >test-file
 $ git add test-file
 $ git commit -m Message. <&-
error: unable to create temporary sha1 filename : No such file or directory

error: Error building trees
 $ 

Dale