{"thread":{"id":"28254","subject":"[PATCH] do not require filters to consume stdin","startedAt":"2011-08-29T20:31:07Z","lastAt":"2011-12-06T03:11:28Z","messageCount":7,"participants":["Joey Hess","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"174492","messageId":"20110829203107.GA4946@gnu.kitenet.net","threadId":"28254","inReplyTo":null,"subject":"[PATCH] do not require filters to consume stdin","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2011-08-29T20:31:07Z","receivedAt":"2011-08-29T20:31:07Z","isPatch":true,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"A clean filter that uses %f to examine a file may not need to consume\nthe entire file content from stdin every time it's run, due to caching\nor other optimisations to support large files.\n\nIgnore the SIGPIPE that may result when writing to the filter\nif it exits without consuming stdin, and do not check that all\ncontent is sent to it. The filter is still required to exit\nsuccessfully, so a crash in the filter should still be handled\ncorrectly.\n---\n\nThere has been discussion before about using clean and smudge filters\nwith %f to handle big files in git, with the file content stored outside\ngit somewhere.  A simplistic clean filter for large files could look\nlike this:\n\n#!/bin/sh\nfile=\"$1\"\nln -f $file ~/.big/$file\necho $file\n\nBut trying to use this will fail on truely large files. For example:\n\n$ ls -l sorta.huge \n-rw-r--r-- 3 joey joey 524288000 Aug 29 15:19 sorta.huge\n$ git add sorta.huge \nbroken pipe  git add sorta.huge\n$ echo $?\n141\n\nThe SIGPIPE occurs because git expects the clean filter to read\nthe full file content from stdin. (Although if the content is small\nenough, a SIGPIPE may not occur.) So the clean filter needs to\nlook like this:\n\n#!/bin/sh\nfile=\"$1\"\ncat >/dev/null\nln -f $file ~/.big/$file\necho $file\n\nBut this means much more work has to be done whenever the clean filter\nis run. Including every time git status is run. So it's currently\nimpractical to use clean/smudge filters like this for large files.\nThis patch should close that gap and allow such filters to be developed.\n\n convert.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 416bf83..3d528c4 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -330,7 +330,7 @@ static int filter_buffer(int in, int out, void *data)\n \t */\n \tstruct child_process child_process;\n \tstruct filter_params *params = (struct filter_params *)data;\n-\tint write_err, status;\n+\tint write_err = 0, status;\n \tconst char *argv[] = { NULL, NULL };\n \n \t/* apply % substitution to cmd */\n@@ -360,9 +360,11 @@ static int filter_buffer(int in, int out, void *data)\n \tif (start_command(&child_process))\n \t\treturn error(\"cannot fork to run external filter %s\", params->cmd);\n \n-\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n+\tsignal(SIGPIPE, SIG_IGN);\n+\twrite_in_full(child_process.in, params->src, params->size);\n \tif (close(child_process.in))\n \t\twrite_err = 1;\n+\tsignal(SIGPIPE, SIG_DFL);\n \tif (write_err)\n \t\terror(\"cannot feed the input to external filter %s\", params->cmd);\n \n-- \n1.7.5.4\n"},{"id":"174519","messageId":"7vobz74yoe.fsf@alter.siamese.dyndns.org","threadId":"28254","inReplyTo":"20110829203107.GA4946@gnu.kitenet.net","subject":"Re: [PATCH] do not require filters to consume stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T22:53:37Z","receivedAt":"2011-08-29T22:53:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joey Hess <joey@kitenet.net> writes:\n\n> There has been discussion before about using clean and smudge filters\n> with %f to handle big files in git, with the file content stored outside\n> git somewhere.  A simplistic clean filter for large files could look\n> like this:\n>\n> #!/bin/sh\n> file=\"$1\"\n> ln -f $file ~/.big/$file\n> echo $file\n\nIsn't this filter already broken if clean request is for a blob contents\nthat is different from what is on the filesystem?  The name %f is passed\nto give the filter a _hint_ on what the path is about (so that the filter\ncan choose to work differently depending on the extension, for example),\nbut the data may or may not come from the filesystem, depending on what is\ncalling the filter, no?\n\nMost notably, renormalize_buffer() would call convert_to_git() on a buffer\nthat is internal, possibly quite different from what is in the working\ntree.\n"},{"id":"174529","messageId":"20110830012029.GA27516@gnu.kitenet.net","threadId":"28254","inReplyTo":"7vobz74yoe.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] do not require filters to consume stdin","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2011-08-30T01:20:29Z","receivedAt":"2011-08-30T01:20:29Z","isPatch":true,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Junio C Hamano wrote:\n> Isn't this filter already broken if clean request is for a blob contents\n> that is different from what is on the filesystem?  The name %f is passed\n> to give the filter a _hint_ on what the path is about (so that the filter\n> can choose to work differently depending on the extension, for example),\n> but the data may or may not come from the filesystem, depending on what is\n> calling the filter, no?\n> \n> Most notably, renormalize_buffer() would call convert_to_git() on a buffer\n> that is internal, possibly quite different from what is in the working\n> tree.\n\nSo during a merge.\n\ngitattributes(5) is not very clear about this, it would probably be good\nto add a caveat there about what %f is not.\n\nThis seems to make it impractical to build the sort of thing described here:\nhttp://lists-archives.org/git/737857-fwd-git-and-large-binaries-a-proposed-solution.html\n\nArguably that thread already reached the same conclusion about using\nsmudge/clean for handling large files, for other reasons. Since I\nalready have something that works without smudge/clean, perhaps I should\ngive up on them.\n\n-- \nsee shy jo\n"},{"id":"180312","messageId":"20111205192930.GA32463@gnu.kitenet.net","threadId":"28254","inReplyTo":"20110829203107.GA4946@gnu.kitenet.net","subject":"hooks that do not consume stdin sometimes crash git with SIGPIPE","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2011-12-05T19:29:30Z","receivedAt":"2011-12-05T19:29:30Z","isPatch":false,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"We had a weird problem where, after moving to a new, faster server,\n\"git push\" would sometimes fail like this:\n\nUnpacking objects: 100% (3/3), done.\nfatal: The remote end hung up unexpectedly\nfatal: The remote end hung up unexpectedly\n\nTurns out that git-receive-pack was dying due to an uncaught SIGPIPE.\nThe SIGPIPE occurred when it tried to write to the pre-receive hook's\nstdin. The pre-receive hook, in this case, was able to do all the checks\nit needed to do[1] without the input, and so did exit(0) without\nconsuming it.\n\nApparently that causes a race. Most of the time, git forks the hook,\nwrites output to the hook, and then the hook runs, ignores it, and exits.\nBut sometimes, on our new faster (and SMP) server, git forked the hook, and\nit ran, and exited, before git got around to writing to it, resulting in\nthe SIGPIPE.\n\nwrite(7, \"c9f98c67d70a1cfeba382ec27d87644a\"..., 100) = -1 EPIPE (Broken pipe)\n--- SIGPIPE (Broken pipe) @ 0 (0) ---\n\nI think git should ignore SIGPIPE when writing to hooks. Otherwise,\nhooks may have to go out of their way to consume all input, and as I've\nseen, the races when they fail to do this can lurk undiscovered.\n\nNote that I encountered this same sort of problem from another direction\n(involving smudge filters) not long ago, and sent a patch, in\n<20110829203107.GA4946@gnu.kitenet.net>. That wasn't applied, and is\nin different code than the case I outlined above.\n\n-- \nsee shy jo\n\n[1] If you're wondering, it only needed to check that the push was\n    coming from a trusted UID. With an untrusted UID, it did further\n    checks that consumed the stdin.\n"},{"id":"180316","messageId":"20111205214351.GA15029@sigill.intra.peff.net","threadId":"28254","inReplyTo":"20111205192930.GA32463@gnu.kitenet.net","subject":"Re: hooks that do not consume stdin sometimes crash git with SIGPIPE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-05T21:43:52Z","receivedAt":"2011-12-05T21:43:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 05, 2011 at 03:29:30PM -0400, Joey Hess wrote:\n\n> We had a weird problem where, after moving to a new, faster server,\n> \"git push\" would sometimes fail like this:\n> \n> Unpacking objects: 100% (3/3), done.\n> fatal: The remote end hung up unexpectedly\n> fatal: The remote end hung up unexpectedly\n> \n> Turns out that git-receive-pack was dying due to an uncaught SIGPIPE.\n> The SIGPIPE occurred when it tried to write to the pre-receive hook's\n> stdin. The pre-receive hook, in this case, was able to do all the checks\n> it needed to do[1] without the input, and so did exit(0) without\n> consuming it.\n\nUgh. Yeah, receive-pack should probably just be ignoring SIGPIPE\nentirely. It checks the return from write() properly where it matters,\nso SIGPIPE is just an annoyance.\n\nI would go so far as to say that git should be ignoring SIGPIPE 99% of\nthe time. We have crap like this sprinkled throughout the code base:\n\n  $ git grep -C1 SIGPIPE\n  builtin/tag.c-  /* When the username signingkey is bad, program could be terminated\n  builtin/tag.c:   * because gpg exits without reading and then write gets SIGPIPE. */\n  builtin/tag.c:  signal(SIGPIPE, SIG_IGN);\n  [...]\n  upload-pack.c-   * If rev-list --stdin encounters an unknown commit, it\n  upload-pack.c:   * terminates, which will cause SIGPIPE in the write loop\n  upload-pack.c-   */\n  upload-pack.c:  sigchain_push(SIGPIPE, SIG_IGN);\n\nbut I find it highly unlikely that they are covering all of the cases.\nYou found one already, and these things can quite often be\nrace-condition heisenbugs.\n\nThe one place where SIGPIPE _is_ useful is for things like \"git log\"\nwhich are just dumping to a pager over stdout. When the pager dies, we\ncan stop bothering to produce output.\n\nIt would be really nice if we could write a sigpipe handler that knew\nwhich fd caused the the signal, and then we could do something like:\n\n  void sigpipe_handler(int sig)\n  {\n          /* If we're writing to a pager over stdout, then there is\n           * little use in writing more; nobody is interested in our\n           * output. */\n          if (get_fd_that_caused_sigpipe() == 1 && pager_in_use)\n                  exit(1);\n\n          /* Otherwise, ignore it, as it's a write to some auxiliary\n           * process and we will be careful about checking the return\n           * code from write(). */\n  }\n\nBut I don't think such a function exists. We could just check\npager_in_use, which would cover most cases (e.g., programs like\nreceive-pack don't use a pager). But it would fail in the case of\nsomething like \"git log\" using an auxiliary process that closes the pipe\nearly. Maybe that would be good enough, though. I dunno.\n\nAnother option is to just ignore SIGPIPE entirely, and convince programs\nlike log to actually bother checking the result of write(). It would be\na slight pain to check every printf() call we make in log-tree.c, but we\ncould do one of:\n\n  1. Make a set of stdio wrapper scripts that exit gracefully on EPIPE.\n     Using the wrapper scripts everywhere is a slight pain, but would\n     work pretty well, and adapts easily to other places like printing\n     lists of refs, etc.\n\n  2. Don't check every printf. After printing each commit, check\n     ferror(stdout) and exit as appropriate. This is a very small amount\n     of code, but you'd need to do it in several places (i.e., anywhere\n     that produces a lot of output).\n\n> I think git should ignore SIGPIPE when writing to hooks. Otherwise,\n> hooks may have to go out of their way to consume all input, and as I've\n> seen, the races when they fail to do this can lurk undiscovered.\n\nYeah, certainly. The question to me is whether we should just stick a\nSIG_IGN in the beginning of receive-pack, or whether we should try to\ndeal with this problem everywhere.\n\nFor example, I suspect the same problem exists in the credential helpers\nI wrote recently. Generally they will read all of their input, but you\ncould do something like:\n\n  [credential \"https://github.com\"]\n          username = peff\n          helper = \"!\n                  f() {\n                          # if we call this helper, we know we want the\n                          # github password,  or else this helper config\n                          # would never have been triggered.  so we\n                          # don't even have to bother reading our stdin.\n\n                          # We only handle getting the password.\n                          test \"$1\" = \"get\" || return\n\n                          # Presumably gpg-agent will ask for and cache\n                          # your gpg password.\n                          p=`gpg -qd --no-tty <~/.github-password.gpg`\n\n                          echo \"password=$p\"\n                  }; f\"\n\nThis can racily fail if our write happens after the helper has already\nfinished (unlikely, but possible).\n\n-Peff\n"},{"id":"180331","messageId":"7vmxb6iim0.fsf@alter.siamese.dyndns.org","threadId":"28254","inReplyTo":"20111205192930.GA32463@gnu.kitenet.net","subject":"Re: hooks that do not consume stdin sometimes crash git with SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-06T01:39:19Z","receivedAt":"2011-12-06T01:39:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joey Hess <joey@kitenet.net> writes:\n\n> We had a weird problem where, after moving to a new, faster server,\n> \"git push\" would sometimes fail like this:\n>\n> Unpacking objects: 100% (3/3), done.\n> fatal: The remote end hung up unexpectedly\n> fatal: The remote end hung up unexpectedly\n>\n> Turns out that git-receive-pack was dying due to an uncaught SIGPIPE.\n\nWhy do you have a hook that is expected to read from receive-pack that\ndoes _not_ read anything from it in the first place? If you do not care\nabout the update status given to pre-receive, shouldn't you be using the\nupdate hook and ignoring the command line parameters instead?\n\nI am not saying this is a user configuration error and there is nothing to\nfix---Git shouldn't get killed merely because of configuration error.\n\nI am wondering if we would want to have a uniform way to tell run_*_hook()\nfunctions that the hook writer explicitly declines to get any input. E.g.\n\"hooks/pre-receive-noinput\" is called instead of \"hooks/pre-receive\" and\nwe do not send any input to it, or something like that.\n"},{"id":"180333","messageId":"20111206031128.GB25805@gnu.kitenet.net","threadId":"28254","inReplyTo":"7vmxb6iim0.fsf@alter.siamese.dyndns.org","subject":"Re: hooks that do not consume stdin sometimes crash git with SIGPIPE","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2011-12-06T03:11:28Z","receivedAt":"2011-12-06T03:11:28Z","isPatch":false,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Junio C Hamano wrote:\n> Why do you have a hook that is expected to read from receive-pack that\n> does _not_ read anything from it in the first place? If you do not care\n> about the update status given to pre-receive, shouldn't you be using the\n> update hook and ignoring the command line parameters instead?\n\nMy hook *does* consume the stdin in one case, but in another case it\ndoes no checks and so can immediately exit. \n\nAlso, I didn't want it to be run once per updated ref as the update hook\nis, since the tests it performs are rather expensive -- loading a perl\nwiki engine in order to check that the changeset contains only changes to\nwiki pages that are allowed based on the wiki's configuration.\n\n-- \nsee shy jo\n"}]}