{"thread":{"id":"34404","subject":"Bug: Failure if stdin is closed.","startedAt":"2013-07-11T15:11:24Z","lastAt":"2013-07-11T17:21:44Z","messageCount":3,"participants":["Dale R. Worley","Thomas Rast","Dale Worley"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"223072","messageId":"201307111511.r6BFBO44010988@freeze.ariadne.com","threadId":"34404","inReplyTo":null,"subject":"Bug: Failure if stdin is closed.","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-07-11T15:11:24Z","receivedAt":"2013-07-11T15:11:24Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"(The original problem and the discussion that ensued is on the\ngit-users mailing list:\nhttps://groups.google.com/forum/#!topic/git-users/lNQ7Cn35EqA)\n\n\"git commit\" (and probably other operations) fail if standard input\n(fd 0) is closed when git starts.  A simple test case follows.  (The\nexecution is version 1.7.7.6, but the code listed below is from a very\nrecent commit.)\n\n\n$ git --version\ngit version 1.7.7.6\n$ git status\n# On branch master\nnothing to commit (working directory clean)\n$ echo This is a test >ffff\n$ git status\n# On branch master\n# Untracked files:\n#   (use \"git add <file>...\" to include in what will be committed)\n#\n#\tffff\nnothing added to commit but untracked files present (use \"git add\" to track)\n$ git add ffff\n$ # The notation \"0<&-\" means \"close standard input (fd 0) in the process that\n$ # executes this command.  It may be specific to the bash shell.\n$ git commit -m xxxx 0<&-\nerror: unable to create temporary sha1 filename : No such file or directory\n\nerror: Error building trees\n$ git status\n# On branch master\n# Changes to be committed:\n#   (use \"git reset HEAD <file>...\" to unstage)\n#\n#\tnew file:   ffff\n#\n$ git commit -m xxxx\n[master 54c146c] xxxx\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 ffff\n$ git status\n# On branch master\n# Your branch is ahead of 'origin/master' by 1 commit.\n#\nnothing to commit (working directory clean)\n$ \n\n\nThe fundamental error is that git_mkstemps_mode() in wrapper.c assumes\nthat if open() is successful, its return value must be >0.  That is\nnot true, because if fd 0 is closed, then open() can successfully\nreturn 0.  I have not tested it, but it looks like the fix is:\n\n int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n {\n\t static const char letters[] =\n\t\t \"abcdefghijklmnopqrstuvwxyz\"\n\t\t \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n\t\t \"0123456789\";\n\t static const int num_letters = 62;\n\t uint64_t value;\n\t struct timeval tv;\n\t char *template;\n\t size_t len;\n\t int fd, count;\n\n\t len = strlen(pattern);\n\n\t if (len < 6 + suffix_len) {\n\t\t errno = EINVAL;\n\t\t return -1;\n\t }\n\n\t if (strncmp(&pattern[len - 6 - suffix_len], \"XXXXXX\", 6)) {\n\t\t errno = EINVAL;\n\t\t return -1;\n\t }\n\n\t /*\n\t  * Replace pattern's XXXXXX characters with randomness.\n\t  * Try TMP_MAX different filenames.\n\t  */\n\t gettimeofday(&tv, NULL);\n\t value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();\n\t template = &pattern[len - 6 - suffix_len];\n\t for (count = 0; count < TMP_MAX; ++count) {\n\t\t uint64_t v = value;\n\t\t /* Fill in the random bits. */\n\t\t template[0] = letters[v % num_letters]; v /= num_letters;\n\t\t template[1] = letters[v % num_letters]; v /= num_letters;\n\t\t template[2] = letters[v % num_letters]; v /= num_letters;\n\t\t template[3] = letters[v % num_letters]; v /= num_letters;\n\t\t template[4] = letters[v % num_letters]; v /= num_letters;\n\t\t template[5] = letters[v % num_letters]; v /= num_letters;\n\n\t\t fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n-\t\t if (fd > 0)\n+\t\t if (fd >= 0)\n\t\t\t return fd;\n\t\t /*\n\t\t  * Fatal error (EPERM, ENOSPC etc).\n\t\t  * It doesn't make sense to loop.\n\t\t  */\n\t\t if (errno != EEXIST)\n\t\t\t break;\n\t\t /*\n\t\t  * This is a random value.  It is only necessary that\n\t\t  * the next TMP_MAX values generated by adding 7777 to\n\t\t  * VALUE are different with (module 2^32).\n\t\t  */\n\t\t value += 7777;\n\t }\n\t /* We return the null string if we can't find a unique file name.  */\n\t pattern[0] = '\\0';\n\t return -1;\n }\n\n\nHowever, when looking at the code, I noticed that few of the functions\nhave comments describing what they do, and none describe their input\nand output values.  In particular, there are no comments specifying\nwhat the error return values are.  This is appalling for a supposedly\nprofessional-quality project!\n\nDale\n"},{"id":"223073","messageId":"87r4f52jjf.fsf@linux-k42r.v.cablecom.net","threadId":"34404","inReplyTo":"201307111511.r6BFBO44010988@freeze.ariadne.com","subject":"Re: Bug: Failure if stdin is closed.","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-07-11T15:39:00Z","receivedAt":"2013-07-11T15:39:00Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"worley@alum.mit.edu (Dale R. Worley) writes:\n\n> (The original problem and the discussion that ensued is on the\n> git-users mailing list:\n> https://groups.google.com/forum/#!topic/git-users/lNQ7Cn35EqA)\n>\n> \"git commit\" (and probably other operations) fail if standard input\n> (fd 0) is closed when git starts.  A simple test case follows.  (The\n> execution is version 1.7.7.6, but the code listed below is from a very\n> recent commit.)\n\nWhile you found a real bug in the use of open() below, I'm not convinced\nthat this scenario is something that should be guarded against.  For\nexample [1] says\n\n  At program start-up, three streams shall be predefined and need not be\n  opened explicitly: standard input (for reading conventional input),\n  standard output (for writing conventional output), and standard error\n  (for writing diagnostic output).\n\nI haven't found a similar clause relating to the file descriptors 0--2,\nbut my reading of the above paragraph is that running a program without\nan open stdin, stdout and stderr is, in fact, not POSIX.\n\nClosing 2 usually has even funkier consequences if the program proceeds\nto open some other file, it ends up as fd 2, and it then dies with an\nerror.  In that sense it might be saner to simply die whenever open()\ngives an FD in the 0..2 range (and we weren't explicitly trying to\nreopen one of them).\n\nMay I ask why you need this, and to what extent this problem cannot be\nsolved by instead redirecting from/to /dev/null?\n\n> The fundamental error is that git_mkstemps_mode() in wrapper.c assumes\n> that if open() is successful, its return value must be >0.  That is\n> not true, because if fd 0 is closed, then open() can successfully\n> return 0.  I have not tested it, but it looks like the fix is:\n[...]\n> \t\t template[5] = letters[v % num_letters]; v /= num_letters;\n>\n> \t\t fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n> -\t\t if (fd > 0)\n> +\t\t if (fd >= 0)\n> \t\t\t return fd;\n> \t\t /*\n> \t\t  * Fatal error (EPERM, ENOSPC etc).\n\nThis is an actual bug, as you say; open() is defined to return -1 on\nerror, not 0.  You can turn this into a patch, and send it as per\nDocumentation/SubmittingPatches.\n\nHowever, does it fully fix the issue you describe?  What if you then run\n'git checkout -F -' to read the message from stdin?\n\n> However, when looking at the code, I noticed that few of the functions\n> have comments describing what they do, and none describe their input\n> and output values.  In particular, there are no comments specifying\n> what the error return values are.  This is appalling for a supposedly\n> professional-quality project!\n\nYou are touching on a sore point of the git code base.  Some\ncontributors have made a point of adding comments where appropriate, so\nwe're improving, but round tuits are in short supply as always.  If you\ncan supply such tuits, they would be appreciated.\n\n\n\n[1]  http://pubs.opengroup.org/onlinepubs/009695399/functions/stdin.html\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"223085","messageId":"201307111721.r6BHLiXD013670@freeze.ariadne.com","threadId":"34404","inReplyTo":"87r4f52jjf.fsf@linux-k42r.v.cablecom.net","subject":"Re: Bug: Failure if stdin is closed.","fromName":"Dale Worley","fromEmail":"worley@c-66-31-108-177.hsd1.ma.comcast.net","sentAt":"2013-07-11T17:21:44Z","receivedAt":"2013-07-11T17:21:44Z","isPatch":false,"sender":{"key":"worley@c-66-31-108-177.hsd1.ma.comcast.net","avatar":null},"body":"> From: Thomas Rast <trast@inf.ethz.ch>\n\n> May I ask why you need this, and to what extent this problem cannot be\n> solved by instead redirecting from/to /dev/null?\n\nThe situation in which the problem arose is described here:\n\n> worley@alum.mit.edu (Dale R. Worley) writes:\n> \n> > (The original problem and the discussion that ensued is on the\n> > git-users mailing list:\n> > https://groups.google.com/forum/#!topic/git-users/lNQ7Cn35EqA)\n\nIt involves invoking git from Apache.  I haven't read up on the\ndetails because I didn't need to in order to debug the problem.\n\n> Closing 2 usually has even funkier consequences if the program proceeds\n> to open some other file, it ends up as fd 2, and it then dies with an\n> error.  In that sense it might be saner to simply die whenever open()\n> gives an FD in the 0..2 range (and we weren't explicitly trying to\n> reopen one of them).\n\nTrue...  But fd 2 may be needed under many unpredictable\ncircumstances.  In regard to fd 0, we can predict that standard input\n(per se) will not be needed, so it's anti-robust to require that it be\nopen for the code to function at all.\n\n> However, does it fully fix the issue you describe?  What if you then run\n> 'git checkout -F -' to read the message from stdin?\n\nObviously, if the git command explicitly requires use of\nstandard-input, then standard-input needs to be open.\n\n> > However, when looking at the code, I noticed that few of the functions\n> > have comments describing what they do, and none describe their input\n> > and output values.  In particular, there are no comments specifying\n> > what the error return values are.  This is appalling for a supposedly\n> > professional-quality project!\n> \n> You are touching on a sore point of the git code base.  Some\n> contributors have made a point of adding comments where appropriate, so\n> we're improving, but round tuits are in short supply as always.  If you\n> can supply such tuits, they would be appreciated.\n\nI will try to put my money where my mouth is.\n\nDale\n"}]}