{"thread":{"id":"4598","subject":"[PATCH 1/4] upload-pack: avoid sending an incomplete pack upon failure","startedAt":"2006-06-21T08:12:00Z","lastAt":"2006-06-21T08:12:00Z","messageCount":1,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"22175","messageId":"7vmzc73p2n.fsf@assigned-by-dhcp.cox.net","threadId":"4598","inReplyTo":null,"subject":"[PATCH 1/4] upload-pack: avoid sending an incomplete pack upon failure","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-21T08:12:00Z","receivedAt":"2006-06-21T08:12:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When the repository on the remote side is corrupted, rev-list\nspawned from upload-pack would die with error, but pack-objects\nthat reads from the rev-list happily created a packfile that can\nbe unpacked by the downloader.  When this happens, the resulting\npackfile is not corrupted and unpacks cleanly, but the list of\nthe objects contained in it is not what the protocol exchange\ncomputed.\n\nThis update makes upload-pack to monitor its subprocesses, and\nwhen either of them dies with error, sends an incomplete pack\ndata to the downloader to cause it to fail.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n * This is about \"upload-pack is unsafe in corrupt repository\"\n   message I sent about 20 hours ago.\n\n upload-pack.c |  177 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n 1 files changed, 163 insertions(+), 14 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 979e583..a9a8f2e 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -5,6 +5,9 @@ #include \"tag.h\"\n #include \"object.h\"\n #include \"commit.h\"\n #include \"exec_cmd.h\"\n+#include <signal.h>\n+#include <sys/poll.h>\n+#include <sys/wait.h>\n \n static const char upload_pack_usage[] = \"git-upload-pack [--strict] [--timeout=nn] <dir>\";\n \n@@ -33,17 +36,20 @@ static int strip(char *line, int len)\n \n static void create_pack_file(void)\n {\n-\tint fd[2];\n-\tpid_t pid;\n+\t/* Pipes between rev-list to pack-objects and pack-objects to us. */\n+\tint lp_pipe[2], pu_pipe[2];\n+\tpid_t pid_rev_list, pid_pack_objects;\n \tint create_full_pack = (nr_our_refs == nr_needs && !nr_has);\n+\tchar data[8193];\n+\tint buffered = -1;\n \n-\tif (pipe(fd) < 0)\n+\tif (pipe(lp_pipe) < 0)\n \t\tdie(\"git-upload-pack: unable to create pipe\");\n-\tpid = fork();\n-\tif (pid < 0)\n+\tpid_rev_list = fork();\n+\tif (pid_rev_list < 0)\n \t\tdie(\"git-upload-pack: unable to fork git-rev-list\");\n \n-\tif (!pid) {\n+\tif (!pid_rev_list) {\n \t\tint i;\n \t\tint args;\n \t\tconst char **argv;\n@@ -60,10 +66,10 @@ static void create_pack_file(void)\n \t\targv = (const char **) p;\n \t\tbuf = xmalloc(args * 45);\n \n-\t\tdup2(fd[1], 1);\n+\t\tdup2(lp_pipe[1], 1);\n \t\tclose(0);\n-\t\tclose(fd[0]);\n-\t\tclose(fd[1]);\n+\t\tclose(lp_pipe[0]);\n+\t\tclose(lp_pipe[1]);\n \t\t*p++ = \"rev-list\";\n \t\t*p++ = use_thin_pack ? \"--objects-edge\" : \"--objects\";\n \t\tif (create_full_pack || MAX_NEEDS <= nr_needs)\n@@ -86,11 +92,154 @@ static void create_pack_file(void)\n \t\texecv_git_cmd(argv);\n \t\tdie(\"git-upload-pack: unable to exec git-rev-list\");\n \t}\n-\tdup2(fd[0], 0);\n-\tclose(fd[0]);\n-\tclose(fd[1]);\n-\texecl_git_cmd(\"pack-objects\", \"--stdout\", NULL);\n-\tdie(\"git-upload-pack: unable to exec git-pack-objects\");\n+\n+\tif (pipe(pu_pipe) < 0)\n+\t\tdie(\"git-upload-pack: unable to create pipe\");\n+\tpid_pack_objects = fork();\n+\tif (pid_pack_objects < 0) {\n+\t\t/* daemon sets things up to ignore TERM */\n+\t\tkill(pid_rev_list, SIGKILL);\n+\t\tdie(\"git-upload-pack: unable to fork git-pack-objects\");\n+\t}\n+\tif (!pid_pack_objects) {\n+\t\tdup2(lp_pipe[0], 0);\n+\t\tdup2(pu_pipe[1], 1);\n+\n+\t\tclose(lp_pipe[0]);\n+\t\tclose(lp_pipe[1]);\n+\t\tclose(pu_pipe[0]);\n+\t\tclose(pu_pipe[1]);\n+\t\texecl_git_cmd(\"pack-objects\", \"--stdout\", NULL);\n+\t\tkill(pid_rev_list, SIGKILL);\n+\t\tdie(\"git-upload-pack: unable to exec git-pack-objects\");\n+\t}\n+\n+\tclose(lp_pipe[0]);\n+\tclose(lp_pipe[1]);\n+\n+\t/* We read from pu_pipe[0] to capture the pack data.\n+\t */\n+\tclose(pu_pipe[1]);\n+\n+\twhile (1) {\n+\t\tconst char *who;\n+\t\tstruct pollfd pfd[2];\n+\t\tpid_t pid;\n+\t\tint status;\n+\t\tssize_t sz;\n+\t\tint pu, pollsize;\n+\n+\t\tpollsize = 0;\n+\t\tpu = -1;\n+\n+\t\tif (0 <= pu_pipe[0]) {\n+\t\t\tpfd[pollsize].fd = pu_pipe[0];\n+\t\t\tpfd[pollsize].events = POLLIN;\n+\t\t\tpu = pollsize;\n+\t\t\tpollsize++;\n+\t\t}\n+\n+\t\tif (pollsize) {\n+\t\t\tif (poll(pfd, pollsize, -1) < 0) {\n+\t\t\t\tif (errno != EINTR) {\n+\t\t\t\t\terror(\"poll failed, resuming: %s\",\n+\t\t\t\t\t      strerror(errno));\n+\t\t\t\t\tsleep(1);\n+\t\t\t\t}\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n+\t\t\t\t/* Data ready; we keep the last byte\n+\t\t\t\t * to ourselves in case we detect\n+\t\t\t\t * broken rev-list, so that we can\n+\t\t\t\t * leave the stream corrupted.  This\n+\t\t\t\t * is unfortunate -- unpack-objects\n+\t\t\t\t * would happily accept a valid pack\n+\t\t\t\t * data with trailing garbage, so\n+\t\t\t\t * appending garbage after we pass all\n+\t\t\t\t * the pack data is not good enough to\n+\t\t\t\t * signal breakage to downstream.\n+\t\t\t\t */\n+\t\t\t\tchar *cp = data;\n+\t\t\t\tssize_t outsz = 0;\n+\t\t\t\tif (0 <= buffered) {\n+\t\t\t\t\t*cp++ = buffered;\n+\t\t\t\t\toutsz++;\n+\t\t\t\t}\n+\t\t\t\tsz = read(pu_pipe[0], cp,\n+\t\t\t\t\t  sizeof(data) - outsz);\n+\t\t\t\tif (0 < sz)\n+\t\t\t\t\t\t;\n+\t\t\t\telse if (sz == 0) {\n+\t\t\t\t\tclose(pu_pipe[0]);\n+\t\t\t\t\tpu_pipe[0] = -1;\n+\t\t\t\t}\n+\t\t\t\telse\n+\t\t\t\t\tgoto fail;\n+\t\t\t\tsz += outsz;\n+\t\t\t\tif (1 < sz) {\n+\t\t\t\t\tbuffered = data[sz-1] & 0xFF;\n+\t\t\t\t\tsz--;\n+\t\t\t\t}\n+\t\t\t\telse\n+\t\t\t\t\tbuffered = -1;\n+\t\t\t\tsz = xwrite(1, data, sz);\n+\t\t\t\tif (sz < 0)\n+\t\t\t\t\tgoto fail;\n+\t\t\t}\n+\t\t}\n+\n+\t\t/* See if the children are still there */\n+\t\tif (pid_rev_list || pid_pack_objects) {\n+\t\t\tpid = waitpid(-1, &status, WNOHANG);\n+\t\t\tif (!pid)\n+\t\t\t\tcontinue;\n+\t\t\twho = ((pid == pid_rev_list) ? \"git-rev-list\" :\n+\t\t\t       (pid == pid_pack_objects) ? \"git-pack-objects\" :\n+\t\t\t       NULL);\n+\t\t\tif (!who) {\n+\t\t\t\tif (pid < 0) {\n+\t\t\t\t\terror(\"git-upload-pack: %s\",\n+\t\t\t\t\t      strerror(errno));\n+\t\t\t\t\tgoto fail;\n+\t\t\t\t}\n+\t\t\t\terror(\"git-upload-pack: we weren't \"\n+\t\t\t\t      \"waiting for %d\", pid);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (!WIFEXITED(status) || WEXITSTATUS(status) > 0) {\n+\t\t\t\terror(\"git-upload-pack: %s died with error.\",\n+\t\t\t\t      who);\n+\t\t\t\tgoto fail;\n+\t\t\t}\n+\t\t\tif (pid == pid_rev_list)\n+\t\t\t\tpid_rev_list = 0;\n+\t\t\tif (pid == pid_pack_objects)\n+\t\t\t\tpid_pack_objects = 0;\n+\t\t\tif (pid_rev_list || pid_pack_objects)\n+\t\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/* both died happily */\n+\t\tif (pollsize)\n+\t\t\tcontinue;\n+\n+\t\t/* flush the data */\n+\t\tif (0 <= buffered) {\n+\t\t\tdata[0] = buffered;\n+\t\t\tsz = xwrite(1, data, 1);\n+\t\t\tif (sz < 0)\n+\t\t\t\tgoto fail;\n+\t\t\tfprintf(stderr, \"flushed.\\n\");\n+\t\t}\n+\t\treturn;\n+\t}\n+ fail:\n+\tif (pid_pack_objects)\n+\t\tkill(pid_pack_objects, SIGKILL);\n+\tif (pid_rev_list)\n+\t\tkill(pid_rev_list, SIGKILL);\n+\tdie(\"git-upload-pack: aborting due to possible repository corruption on the remote side.\");\n }\n \n static int got_sha1(char *hex, unsigned char *sha1)\n-- \n1.4.0.gbb3f\n"}]}