# [PATCH 0/2] open() error checking 16 messages from 2013-07-12 to 2013-07-18. Participants: Thomas Rast, Junio C Hamano, Drew Northup, Dale R. Worley, Eric Sunshine. Thread: https://gitlist.dev/t/34415 ## Thomas Rast, 2013-07-12 08:58 Subject: [PATCH 0/2] open() error checking Message-ID: URL: https://gitlist.dev/e/cover.1373618940.git.trast%40inf.ethz.ch ``` #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 ``` ## Thomas Rast, 2013-07-12 08:58 Subject: [PATCH 1/2] git_mkstemps: correctly test return value of open() Message-ID: <9af38018d55c95a6807d305bb3a088e48916baac.1373618940.git.trast@inf.ethz.ch> URL: https://gitlist.dev/e/9af38018d55c95a6807d305bb3a088e48916baac.1373618940.git.trast%40inf.ethz.ch In-Reply-To: ``` From: "Dale R. Worley" 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 --- 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 ``` ## Thomas Rast, 2013-07-12 08:58 Subject: [PATCH 2/2] run-command: dup_devnull(): guard against syscalls failing Message-ID: <0f1e919ab5886d00d6956499cf5ed3e064033f11.1373618940.git.trast@inf.ethz.ch> URL: https://gitlist.dev/e/0f1e919ab5886d00d6956499cf5ed3e064033f11.1373618940.git.trast%40inf.ethz.ch In-Reply-To: ``` dup_devnull() did not check the return values of open() and dup2(). Fix this omission. Signed-off-by: Thomas Rast --- 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 ``` ## Junio C Hamano, 2013-07-12 17:29 Subject: Re: [PATCH 0/2] open() error checking Message-ID: <7vtxjzlmaf.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtxjzlmaf.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Thomas Rast 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. ``` ## Thomas Rast, 2013-07-16 09:25 Subject: Re: [PATCH 0/2] open() error checking Message-ID: <87hafukga9.fsf@linux-k42r.v.cablecom.net> URL: https://gitlist.dev/e/87hafukga9.fsf%40linux-k42r.v.cablecom.net In-Reply-To: <7vtxjzlmaf.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Thomas Rast 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 ``` ## Thomas Rast, 2013-07-16 09:37 Subject: Re: [PATCH 1/2] git_mkstemps: correctly test return value of open() Message-ID: <878v16kfqy.fsf@linux-k42r.v.cablecom.net> URL: https://gitlist.dev/e/878v16kfqy.fsf%40linux-k42r.v.cablecom.net In-Reply-To: <9af38018d55c95a6807d305bb3a088e48916baac.1373618940.git.trast@inf.ethz.ch> ``` Thomas Rast writes: > From: "Dale R. Worley" > > 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 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 ``` ## Junio C Hamano, 2013-07-17 19:29 Subject: Re: [PATCH 1/2] git_mkstemps: correctly test return value of open() Message-ID: <7v38rd6l3j.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v38rd6l3j.fsf%40alter.siamese.dyndns.org In-Reply-To: <878v16kfqy.fsf@linux-k42r.v.cablecom.net> ``` Thomas Rast writes: > Thomas Rast writes: > >> From: "Dale R. Worley" >> >> 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 > > 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). ``` ## Drew Northup, 2013-07-18 12:32 Subject: Re: [PATCH 1/2] git_mkstemps: correctly test return value of open() Message-ID: <51E7E05E.4000201@gmail.com> URL: https://gitlist.dev/e/51E7E05E.4000201%40gmail.com In-Reply-To: <7v38rd6l3j.fsf@alter.siamese.dyndns.org> ``` 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 writes: >> Thomas Rast writes: >>> From: "Dale R. Worley" >>> >>> 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 >>> 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 ``` ## Junio C Hamano, 2013-07-18 17:46 Subject: Re: [PATCH 1/2] git_mkstemps: correctly test return value of open() Message-ID: <7v4nbr4v7m.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4nbr4v7m.fsf%40alter.siamese.dyndns.org In-Reply-To: <51E7E05E.4000201@gmail.com> ``` Drew Northup 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 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 writes: >>> Thomas Rast writes: >>>> From: "Dale R. Worley" >>>> >>>> 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 > >>>> 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 ``` ## Junio C Hamano, 2013-07-18 17:47 Subject: Re: [PATCH 1/2] git_mkstemps: correctly test return value of open() Message-ID: <7vzjtj3gl4.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzjtj3gl4.fsf%40alter.siamese.dyndns.org In-Reply-To: <7v4nbr4v7m.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Drew Northup 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 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 writes: >>>> Thomas Rast writes: >>>>> From: "Dale R. Worley" >>>>> >>>>> 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 >> >>>>> 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 ``` ## Dale R. Worley, 2013-07-18 20:32 Subject: Re: [PATCH 1/2] git_mkstemps: correctly test return value of open() Message-ID: <201307182032.r6IKWtWC016218@freeze.ariadne.com> URL: https://gitlist.dev/e/201307182032.r6IKWtWC016218%40freeze.ariadne.com In-Reply-To: <7v4nbr4v7m.fsf@alter.siamese.dyndns.org> ``` 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 + +/* 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 ``` ## Eric Sunshine, 2013-07-18 20:49 Subject: Re: [PATCH 1/2] git_mkstemps: correctly test return value of open() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRyRq3depm%2BeBzZp-nRa2iPKbK-JU8cXgZdzQnCduFTMA%40mail.gmail.com In-Reply-To: <201307182032.r6IKWtWC016218@freeze.ariadne.com> ``` On Thu, Jul 18, 2013 at 4:32 PM, Dale R. Worley 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? ``` ## Junio C Hamano, 2013-07-18 20:54 Subject: Re: [PATCH 1/2] git_mkstemps: correctly test return value of open() Message-ID: URL: https://gitlist.dev/e/CAPc5daVVDCHqjyDV3zYVV33EFYjea7ge84%2BCE%3DM%3DQXagxnHd-A%40mail.gmail.com In-Reply-To: <201307182032.r6IKWtWC016218@freeze.ariadne.com> ``` On Thu, Jul 18, 2013 at 1:32 PM, Dale R. Worley 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 ``` ## Dale R. Worley, 2013-07-18 22:46 Subject: Re: [PATCH 1/2] git_mkstemps: correctly test return value of open() Message-ID: <201307182246.r6IMkDiW021930@freeze.ariadne.com> URL: https://gitlist.dev/e/201307182246.r6IMkDiW021930%40freeze.ariadne.com In-Reply-To: ``` > From: Junio C Hamano > > 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 ``` ## Junio C Hamano, 2013-07-18 23:23 Subject: Re: [PATCH 1/2] git_mkstemps: correctly test return value of open() Message-ID: <7vhafr1mhy.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhafr1mhy.fsf%40alter.siamese.dyndns.org In-Reply-To: <201307182246.r6IMkDiW021930@freeze.ariadne.com> ``` worley@alum.mit.edu (Dale R. Worley) writes: >> From: Junio C Hamano >> >> 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 ``` ## Dale R. Worley, 2013-07-18 23:29 Subject: Re: [PATCH 1/2] git_mkstemps: correctly test return value of open() Message-ID: <201307182329.r6INTP7A022917@freeze.ariadne.com> URL: https://gitlist.dev/e/201307182329.r6INTP7A022917%40freeze.ariadne.com In-Reply-To: <7vhafr1mhy.fsf@alter.siamese.dyndns.org> ``` > From: Junio C Hamano > > +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 ```