{"thread":{"id":"34415","subject":"[PATCH 0/2] open() error checking","startedAt":"2013-07-12T08:58:34Z","lastAt":"2013-07-18T23:29:25Z","messageCount":16,"participants":["Thomas Rast","Junio C Hamano","Drew Northup","Dale R. Worley","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"223152","messageId":"cover.1373618940.git.trast@inf.ethz.ch","threadId":"34415","inReplyTo":null,"subject":"[PATCH 0/2] open() error checking","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-07-12T08:58:34Z","receivedAt":"2013-07-12T08:58:34Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"#1 is Dale's suggested change. Dale, to include it we'd need your\nSigned-off-by as per Documentation/SubmittingPatches.\n\n#2 is a similar error-checking fix; I reviewed 'git grep \"\\bopen\\b\"'\nand found one case where the return value was obviously not tested.\nThe corresponding Windows code path has the same problem, but I dare\nnot touch it; perhaps someone from the Windows side can look into it?\n\nI originally had a four-patch series to open 0/1/2 from /dev/null, but\nthen I noticed that this was shot down in 2008:\n\n http://thread.gmane.org/gmane.comp.version-control.git/93605/focus=93896\n\nDo you want to resurrect this?\n\nThe worst part about it is that because we don't have a stderr to rely\non, we can't simply die(\"stop playing mind games\").\n\n\nDale R. Worley (1):\n git_mkstemps: correctly test return value of open()\n\nThomas Rast (1):\n run-command: dup_devnull(): guard against syscalls failing\n\n run-command.c | 5 ++++-\n wrapper.c | 2 +-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\n-- \n1.8.3.2.998.g1d087bc\n"},{"id":"223153","messageId":"9af38018d55c95a6807d305bb3a088e48916baac.1373618940.git.trast@inf.ethz.ch","threadId":"34415","inReplyTo":"cover.1373618940.git.trast@inf.ethz.ch","subject":"[PATCH 1/2] git_mkstemps: correctly test return value of open()","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-07-12T08:58:35Z","receivedAt":"2013-07-12T08:58:35Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"From: \"Dale R. Worley\" \n\nopen() returns -1 on failure, and indeed 0 is a possible success value\nif the user closed stdin in our process. Fix the test.\n\nSigned-off-by: Thomas Rast \n---\n wrapper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex dd7ecbb..6a015de 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n \n \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n-\t\tif (fd > 0)\n+\t\tif (fd >= 0)\n \t\t\treturn fd;\n \t\t/*\n \t\t * Fatal error (EPERM, ENOSPC etc).\n-- \n1.8.3.2.998.g1d087bc\n"},{"id":"223154","messageId":"0f1e919ab5886d00d6956499cf5ed3e064033f11.1373618940.git.trast@inf.ethz.ch","threadId":"34415","inReplyTo":"cover.1373618940.git.trast@inf.ethz.ch","subject":"[PATCH 2/2] run-command: dup_devnull(): guard against syscalls failing","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-07-12T08:58:36Z","receivedAt":"2013-07-12T08:58:36Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"dup_devnull() did not check the return values of open() and dup2().\nFix this omission.\n\nSigned-off-by: Thomas Rast \n---\n run-command.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex aece872..1b7f88e 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -76,7 +76,10 @@ static inline void close_pair(int fd[2])\n static inline void dup_devnull(int to)\n {\n \tint fd = open(\"/dev/null\", O_RDWR);\n-\tdup2(fd, to);\n+\tif (fd < 0)\n+\t\tdie_errno(_(\"open /dev/null failed\"));\n+\tif (dup2(fd, to) < 0)\n+\t\tdie_errno(_(\"dup2(%d,%d) failed\"), fd, to);\n \tclose(fd);\n }\n #endif\n-- \n1.8.3.2.998.g1d087bc\n"},{"id":"223184","messageId":"7vtxjzlmaf.fsf@alter.siamese.dyndns.org","threadId":"34415","inReplyTo":"cover.1373618940.git.trast@inf.ethz.ch","subject":"Re: [PATCH 0/2] open() error checking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-12T17:29:12Z","receivedAt":"2013-07-12T17:29:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> #1 is Dale's suggested change. Dale, to include it we'd need your\n> Signed-off-by as per Documentation/SubmittingPatches.\n>\n> #2 is a similar error-checking fix; I reviewed 'git grep \"\\bopen\\b\"'\n> and found one case where the return value was obviously not tested.\n> The corresponding Windows code path has the same problem, but I dare\n> not touch it; perhaps someone from the Windows side can look into it?\n>\n> I originally had a four-patch series to open 0/1/2 from /dev/null, but\n> then I noticed that this was shot down in 2008:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/93605/focus=93896\n\nThe way I recall the thread was not \"shot down\" but more like\n\"fizzled out without seeing a clear consensus\". As a normal POSIX\nprogram, we do rely on fd#2 connected to an error stream, and I do\nagree with the general sentiment of that old thread that it is very\nwrong for warning() or die() to write to a pipe or file descriptor\nwe opened for some other purpose, corrupting the destination.\n\nI briefly wondered if we can do the sanity check lazily (e.g. upon\nfirst warning() see of fd#2 is open and otherwise die silently), but\nwe may open a fd (e.g. to create a new loose object) that may happen\nto grab fd#2 and then it is too late for us to do anything about it,\nso...\n\n> Do you want to resurrect this?\n>\n> The worst part about it is that because we don't have a stderr to rely\n> on, we can't simply die(\"stop playing mind games\").\n\nRight.\n"},{"id":"223514","messageId":"87hafukga9.fsf@linux-k42r.v.cablecom.net","threadId":"34415","inReplyTo":"7vtxjzlmaf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/2] open() error checking","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-07-16T09:25:34Z","receivedAt":"2013-07-16T09:25:34Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Rast writes:\n>\n>> I originally had a four-patch series to open 0/1/2 from /dev/null, but\n>> then I noticed that this was shot down in 2008:\n>>\n>> http://thread.gmane.org/gmane.comp.version-control.git/93605/focus=93896\n>\n> The way I recall the thread was not \"shot down\" but more like\n> \"fizzled out without seeing a clear consensus\". As a normal POSIX\n> program, we do rely on fd#2 connected to an error stream, and I do\n> agree with the general sentiment of that old thread that it is very\n> wrong for warning() or die() to write to a pipe or file descriptor\n> we opened for some other purpose, corrupting the destination.\n>\n> I briefly wondered if we can do the sanity check lazily (e.g. upon\n> first warning() see of fd#2 is open and otherwise die silently), but\n> we may open a fd (e.g. to create a new loose object) that may happen\n> to grab fd#2 and then it is too late for us to do anything about it,\n> so...\n\nI think we'd have to do it on startup. Since we do many things already,\na few extra dup calls should hardly matter.\n\nI'll send the patches in reply in a minute, I had them lying around\nalready. But if you (again) decide that it's not worth it, I don't care\ntoo deeply.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"223517","messageId":"878v16kfqy.fsf@linux-k42r.v.cablecom.net","threadId":"34415","inReplyTo":"9af38018d55c95a6807d305bb3a088e48916baac.1373618940.git.trast@inf.ethz.ch","subject":"Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-07-16T09:37:09Z","receivedAt":"2013-07-16T09:37:09Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast writes:\n\n> From: \"Dale R. Worley\" \n>\n> open() returns -1 on failure, and indeed 0 is a possible success value\n> if the user closed stdin in our process. Fix the test.\n>\n> Signed-off-by: Thomas Rast \n\nI see you have this in 'pu' without Dale's signoff. I'm guessing\n(IANAL) that it's too small to be copyrighted and anyway there is only\nway to fix it, but maybe Dale can \"sign off\" just to be safe, anyway?\n\n> wrapper.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/wrapper.c b/wrapper.c\n> index dd7ecbb..6a015de 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n> \t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n> \n> \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n> -\t\tif (fd > 0)\n> +\t\tif (fd >= 0)\n> \t\t\treturn fd;\n> \t\t/*\n> \t\t * Fatal error (EPERM, ENOSPC etc).\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"223601","messageId":"7v38rd6l3j.fsf@alter.siamese.dyndns.org","threadId":"34415","inReplyTo":"878v16kfqy.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-17T19:29:52Z","receivedAt":"2013-07-17T19:29:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Thomas Rast writes:\n>\n>> From: \"Dale R. Worley\" \n>>\n>> open() returns -1 on failure, and indeed 0 is a possible success value\n>> if the user closed stdin in our process. Fix the test.\n>>\n>> Signed-off-by: Thomas Rast \n>\n> I see you have this in 'pu' without Dale's signoff. I'm guessing\n> (IANAL) that it's too small to be copyrighted and anyway there is only\n> way to fix it, but maybe Dale can \"sign off\" just to be safe, anyway?\n\nYup, that is a good idea. Thanks.\n\n>\n>> wrapper.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/wrapper.c b/wrapper.c\n>> index dd7ecbb..6a015de 100644\n>> --- a/wrapper.c\n>> +++ b/wrapper.c\n>> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>> \t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n>> \n>> \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n>> -\t\tif (fd > 0)\n>> +\t\tif (fd >= 0)\n>> \t\t\treturn fd;\n>> \t\t/*\n>> \t\t * Fatal error (EPERM, ENOSPC etc).\n"},{"id":"223645","messageId":"51E7E05E.4000201@gmail.com","threadId":"34415","inReplyTo":"7v38rd6l3j.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()","fromName":"Drew Northup","fromEmail":"n1xim.email@gmail.com","sentAt":"2013-07-18T12:32:30Z","receivedAt":"2013-07-18T12:32:30Z","isPatch":true,"sender":{"key":"n1xim.email@gmail.com","avatar":null},"body":"I presume that I should apply this change to my porting of \ngit_mkstemps_mode() to tig. If there are no complaints about this for a \ncouple of days I will do so.\n\nREF: $gmane/229961\n\nOn 07/17/2013 03:29 PM, Junio C Hamano wrote:\n> Thomas Rast writes:\n>> Thomas Rast writes:\n>>> From: \"Dale R. Worley\"\n>>>\n>>> open() returns -1 on failure, and indeed 0 is a possible success value\n>>> if the user closed stdin in our process. Fix the test.\n>>>\n>>> Signed-off-by: Thomas Rast\n\n>>> wrapper.c | 2 +-\n>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/wrapper.c b/wrapper.c\n>>> index dd7ecbb..6a015de 100644\n>>> --- a/wrapper.c\n>>> +++ b/wrapper.c\n>>> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>>> \t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n>>>\n>>> \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n>>> -\t\tif (fd> 0)\n>>> +\t\tif (fd>= 0)\n>>> \t\t\treturn fd;\n>>> \t\t/*\n>>> \t\t * Fatal error (EPERM, ENOSPC etc).\n\n\n--\n-Drew Northup\n--------------------------------------------------------------\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"223670","messageId":"7v4nbr4v7m.fsf@alter.siamese.dyndns.org","threadId":"34415","inReplyTo":"51E7E05E.4000201@gmail.com","subject":"Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T17:46:37Z","receivedAt":"2013-07-18T17:46:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Drew Northup writes:\n\n> I presume that I should apply this change to my porting of\n> git_mkstemps_mode() to tig. If there are no complaints about this for\n> a couple of days I will do so.\n\nHmph, Thomas and I were actually asking you to give us\n\n\tSigned-off-by: Drew Northup \n\nfor the patch in question. If tig has the same issue, applying that\nsame patch there may make sense, but that is an independent issue.\n\nThanks.\n\n>\n> REF: $gmane/229961\n>\n> On 07/17/2013 03:29 PM, Junio C Hamano wrote:\n>> Thomas Rast writes:\n>>> Thomas Rast writes:\n>>>> From: \"Dale R. Worley\"\n>>>>\n>>>> open() returns -1 on failure, and indeed 0 is a possible success value\n>>>> if the user closed stdin in our process. Fix the test.\n>>>>\n>>>> Signed-off-by: Thomas Rast\n>\n>>>> wrapper.c | 2 +-\n>>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/wrapper.c b/wrapper.c\n>>>> index dd7ecbb..6a015de 100644\n>>>> --- a/wrapper.c\n>>>> +++ b/wrapper.c\n>>>> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>>>> \t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n>>>>\n>>>> \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n>>>> -\t\tif (fd> 0)\n>>>> +\t\tif (fd>= 0)\n>>>> \t\t\treturn fd;\n>>>> \t\t/*\n>>>> \t\t * Fatal error (EPERM, ENOSPC etc).\n>\n>\n> --\n> -Drew Northup\n> --------------------------------------------------------------\n> \"As opposed to vegetable or mineral error?\"\n> -John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"223671","messageId":"7vzjtj3gl4.fsf@alter.siamese.dyndns.org","threadId":"34415","inReplyTo":"7v4nbr4v7m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T17:47:51Z","receivedAt":"2013-07-18T17:47:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Drew Northup writes:\n>\n>> I presume that I should apply this change to my porting of\n>> git_mkstemps_mode() to tig. If there are no complaints about this for\n>> a couple of days I will do so.\n>\n> Hmph, Thomas and I were actually asking you to give us\n>\n> \tSigned-off-by: Drew Northup \n\nGaahhh, I need a bit more caffeine. Somehow I mixed up Dale and\nDrew.\n\nSorry for the noise. Please ignore.\n\n\n> for the patch in question. If tig has the same issue, applying that\n> same patch there may make sense, but that is an independent issue.\n>\n> Thanks.\n>\n>>\n>> REF: $gmane/229961\n>>\n>> On 07/17/2013 03:29 PM, Junio C Hamano wrote:\n>>> Thomas Rast writes:\n>>>> Thomas Rast writes:\n>>>>> From: \"Dale R. Worley\"\n>>>>>\n>>>>> open() returns -1 on failure, and indeed 0 is a possible success value\n>>>>> if the user closed stdin in our process. Fix the test.\n>>>>>\n>>>>> Signed-off-by: Thomas Rast\n>>\n>>>>> wrapper.c | 2 +-\n>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>>>\n>>>>> diff --git a/wrapper.c b/wrapper.c\n>>>>> index dd7ecbb..6a015de 100644\n>>>>> --- a/wrapper.c\n>>>>> +++ b/wrapper.c\n>>>>> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>>>>> \t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n>>>>>\n>>>>> \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n>>>>> -\t\tif (fd> 0)\n>>>>> +\t\tif (fd>= 0)\n>>>>> \t\t\treturn fd;\n>>>>> \t\t/*\n>>>>> \t\t * Fatal error (EPERM, ENOSPC etc).\n>>\n>>\n>> --\n>> -Drew Northup\n>> --------------------------------------------------------------\n>> \"As opposed to vegetable or mineral error?\"\n>> -John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"223696","messageId":"201307182032.r6IKWtWC016218@freeze.ariadne.com","threadId":"34415","inReplyTo":"7v4nbr4v7m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-07-18T20:32:55Z","receivedAt":"2013-07-18T20:32:55Z","isPatch":true,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"I've been looking into writing a proper test for this patch. My first\nattempt tests the symptom that was seen initially, that \"git commit\"\nfails if fd 0 is closed.\n\nOne problem is how to arrange for fd 0 to be closed. I could use the\nbash redirection \"<&-\", but I think you want to be more portable than\nthat. This version uses execvp() inside a small C program, and\nexecvp() is a Posix function.\n\nI've tested that this test does what it should: If you remove the\nfix, \"fd >= 0\", the test fails. If you then remove \"test-close-fd-0\"\nfrom before \"git init\" in the test, the test is nullified and succeeds\nagain.\n\nHere is the diff. What do people think of it?\n\ndiff --git a/Makefile b/Makefile\nindex 0600eb4..6b410f5 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -557,6 +557,7 @@ X =\n PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))\n \n TEST_PROGRAMS_NEED_X += test-chmtime\n+TEST_PROGRAMS_NEED_X += test-close-fd-0\n TEST_PROGRAMS_NEED_X += test-ctype\n TEST_PROGRAMS_NEED_X += test-date\n TEST_PROGRAMS_NEED_X += test-delta\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 986b2a8..6a31103 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -25,6 +25,13 @@ test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints file\n \tgrep \"cannotwrite/test\" err\n '\n \n+test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' '\n+\tgit init &&\n+\techo Test. >test-file &&\n+\tgit add test-file &&\n+\ttest-close-fd-0 git commit -m Message.\n+'\n+\n test_expect_success 'check for a bug in the regex routines' '\n \t# if this test fails, re-build git with NO_REGEX=1\n \ttest-regex\ndiff --git a/test-close-fd-0.c b/test-close-fd-0.c\nnew file mode 100644\nindex 0000000..3745c34\n--- /dev/null\n+++ b/test-close-fd-0.c\n@@ -0,0 +1,14 @@\n+#include \n+\n+/* Close file descriptor 0 (which is standard-input), then execute the\n+ * remainder of the command line as a command. */\n+\n+int main(int argc, char **argv)\n+{\n+\t/* Close fd 0. */\n+\tclose(0);\n+\t/* Execute the requested command. */\n+\texecvp(argv[1], &argv[1]);\n+\t/* If execve() failed, return an error. */\n+\treturn 1;\n+}\ndiff --git a/wrapper.c b/wrapper.c\nindex dd7ecbb..6a015de 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n \n \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n-\t\tif (fd > 0)\n+\t\tif (fd >= 0)\n \t\t\treturn fd;\n \t\t/*\n \t\t * Fatal error (EPERM, ENOSPC etc).\n\nDale\n"},{"id":"223698","messageId":"CAPig+cRyRq3depm+eBzZp-nRa2iPKbK-JU8cXgZdzQnCduFTMA@mail.gmail.com","threadId":"34415","inReplyTo":"201307182032.r6IKWtWC016218@freeze.ariadne.com","subject":"Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-18T20:49:47Z","receivedAt":"2013-07-18T20:49:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 18, 2013 at 4:32 PM, Dale R. Worley wrote:\n> I've been looking into writing a proper test for this patch. My first\n> attempt tests the symptom that was seen initially, that \"git commit\"\n> fails if fd 0 is closed.\n>\n> One problem is how to arrange for fd 0 to be closed. I could use the\n> bash redirection \"<&-\", but I think you want to be more portable than\n> that. This version uses execvp() inside a small C program, and\n> execvp() is a Posix function.\n\n\"<&-\" is POSIX.\n\nhttp://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_05\n\n> I've tested that this test does what it should: If you remove the\n> fix, \"fd >= 0\", the test fails. If you then remove \"test-close-fd-0\"\n> from before \"git init\" in the test, the test is nullified and succeeds\n> again.\n>\n> Here is the diff. What do people think of it?\n"},{"id":"223701","messageId":"CAPc5daVVDCHqjyDV3zYVV33EFYjea7ge84+CE=M=QXagxnHd-A@mail.gmail.com","threadId":"34415","inReplyTo":"201307182032.r6IKWtWC016218@freeze.ariadne.com","subject":"Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T20:54:35Z","receivedAt":"2013-07-18T20:54:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Thu, Jul 18, 2013 at 1:32 PM, Dale R. Worley wrote:\n> I've been looking into writing a proper test for this patch. My first\n> attempt tests the symptom that was seen initially, that \"git commit\"\n> fails if fd 0 is closed.\n>\n> One problem is how to arrange for fd 0 to be closed. I could use the\n> bash redirection \"<&-\", but I think you want to be more portable than\n> that.\n\nThat's just a plain-vanilla part of POSIX shell behaviour, no?\n\nhttp://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_05\n"},{"id":"223717","messageId":"201307182246.r6IMkDiW021930@freeze.ariadne.com","threadId":"34415","inReplyTo":"CAPc5daVVDCHqjyDV3zYVV33EFYjea7ge84+CE=M=QXagxnHd-A@mail.gmail.com","subject":"Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-07-18T22:46:13Z","receivedAt":"2013-07-18T22:46:13Z","isPatch":true,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"> From: Junio C Hamano \n> \n> That's just a plain-vanilla part of POSIX shell behaviour, no?\n> \n> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_05\n\n\"Close standard input\" is so weird I never thought it was Posix. In\nthat case, we can eliminate the C helper program:\n\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 986b2a8..d427f3a 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -25,6 +25,13 @@ test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints file\n \tgrep \"cannotwrite/test\" err\n '\n \n+test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' '\n+\tgit init &&\n+\techo Test. >test-file &&\n+\tgit add test-file &&\n+\tgit commit -m Message. <&-\n+'\n+\n test_expect_success 'check for a bug in the regex routines' '\n \t# if this test fails, re-build git with NO_REGEX=1\n \ttest-regex\ndiff --git a/wrapper.c b/wrapper.c\nindex dd7ecbb..6a015de 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n \n \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n-\t\tif (fd > 0)\n+\t\tif (fd >= 0)\n \t\t\treturn fd;\n \t\t/*\n \t\t * Fatal error (EPERM, ENOSPC etc).\n\nIs this a sensible place to put this test?\n\nDale\n"},{"id":"223720","messageId":"7vhafr1mhy.fsf@alter.siamese.dyndns.org","threadId":"34415","inReplyTo":"201307182246.r6IMkDiW021930@freeze.ariadne.com","subject":"Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T23:23:05Z","receivedAt":"2013-07-18T23:23:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"worley@alum.mit.edu (Dale R. Worley) writes:\n\n>> From: Junio C Hamano \n>> \n>> That's just a plain-vanilla part of POSIX shell behaviour, no?\n>> \n>> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_05\n>\n> \"Close standard input\" is so weird I never thought it was Posix. In\n> that case, we can eliminate the C helper program:\n>\n> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\n> index 986b2a8..d427f3a 100755\n> --- a/t/t0070-fundamental.sh\n> +++ b/t/t0070-fundamental.sh\n> @@ -25,6 +25,13 @@ test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints file\n> \tgrep \"cannotwrite/test\" err\n> '\n> \n> +test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' '\n> +\tgit init &&\n> +\techo Test. >test-file &&\n> +\tgit add test-file &&\n> +\tgit commit -m Message. <&-\n> +'\n> +\n\nYup. I wonder how it would fail without the fix, though ;-)\n\n> test_expect_success 'check for a bug in the regex routines' '\n> \t# if this test fails, re-build git with NO_REGEX=1\n> \ttest-regex\n> diff --git a/wrapper.c b/wrapper.c\n> index dd7ecbb..6a015de 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n> \t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n> \n> \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n> -\t\tif (fd > 0)\n> +\t\tif (fd >= 0)\n> \t\t\treturn fd;\n> \t\t/*\n> \t\t * Fatal error (EPERM, ENOSPC etc).\n>\n> Is this a sensible place to put this test?\n>\n> Dale\n"},{"id":"223721","messageId":"201307182329.r6INTP7A022917@freeze.ariadne.com","threadId":"34415","inReplyTo":"7vhafr1mhy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-07-18T23:29:25Z","receivedAt":"2013-07-18T23:29:25Z","isPatch":true,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"> From: Junio C Hamano \n\n> > +test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' '\n> > +\tgit init &&\n> > +\techo Test. >test-file &&\n> > +\tgit add test-file &&\n> > +\tgit commit -m Message. <&-\n> > +'\n> > +\n> \n> Yup. I wonder how it would fail without the fix, though ;-)\n\nEh, what? You could run it and see. The test script system will just\nsay \"not ok\" for this test. If you execute those commands from a\nshell, you see:\n\n $ git init\nInitialized empty Git repository in /common/not-replicated/worley/temp/1/.git/\n $ echo Test. >test-file\n $ git add test-file\n $ git commit -m Message. <&-\nerror: unable to create temporary sha1 filename : No such file or directory\n\nerror: Error building trees\n $ \n\nDale\n"}]}