{"thread":{"id":"50777","subject":"git on AIX: daemon.c & t5570-git-daemon.sh","startedAt":"2019-03-19T09:06:01Z","lastAt":"2019-04-22T19:52:30Z","messageCount":2,"participants":["REIX, Tony","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"371892","messageId":"DB7PR02MB46634F68900E0F81B0110AF586400@DB7PR02MB4663.eurprd02.prod.outlook.com","threadId":"50777","inReplyTo":"DB7PR02MB466360C2CBC44AE5FEC22CC586470@DB7PR02MB4663.eurprd02.prod.outlook.com","subject":"git on AIX: daemon.c & t5570-git-daemon.sh","fromName":"REIX, Tony","fromEmail":"tony.reix@atos.net","sentAt":"2019-03-19T09:05:49Z","receivedAt":"2019-03-19T09:06:01Z","isPatch":false,"sender":{"key":"tony.reix@atos.net","avatar":null},"body":"Hi,\n\nWhen testing version 2.21.0 of git on AIX (6.1 & 7.2), I have found an issue with daemon.c and test t5570-git-daemon.sh : within test 4, the child_handler() code gets crazy and calls itself recursively till the process crashes. We do not have a clear idea why this issue occurs. Maybe that this issue also appears on other operating  systems.\n\nThis code:\n\nstatic void child_handler(int signo)\n{\n        signal(SIGCHLD, child_handler);\n}\nis the root cause and it appeared in 2008, for SysV :\n    https://git.kernel.org/pub/scm/git/git.git/commit/daemon.c?id=bd7b371e9c2aeb0aaf228dc1655e8d04fca6f797\n\nI suggest to use the attached patch  git-2.21.0-SIGCHLD-handler-v3.patch, so that this issue on AIX disappears.\nMoreover, this code may also be useful for other operating systems. Someone should tests it on Solaris.\n\nI have checked that this patch is OK (build & test t5570-git-daemon.sh) on Linux/Power (Fedora 28) with GCC (v8.1).\n\nI've also checked on Linux/Power that the 21 tests of t5570-git-daemon.sh are OK when one replaces the existing code using:\n    signal(SIGCHLD, child_handler);\nby the code dedicated to AIX.\nI mean to say that this sigaction(SIGCHLD, &sa, NULL) code could also replace on Linux the current potentially-dangerous code based on signal(SIGCHLD, child_handler).\n\nRegards,\n\nTony\n \n\n\ntony.reix@atos.net\n\nATOS / Bull SAS\nATOS Expert\nIBM Coop Architect & Technical Leader\n\nOffice : +33 (0) 4 76 29 72 67\n1 rue de Provence - 38432 Échirolles - France\nwww.atos.net            \n\n--- ./daemon.c.ORIGIN\t2019-03-18 17:53:51 +0100\n+++ ./daemon.c\t2019-03-18 18:00:16 +0100\n@@ -943,8 +943,11 @@\n \t * Otherwise empty handler because systemcalls will get interrupted\n \t * upon signal receipt\n \t * SysV needs the handler to be rearmed\n+\t * AIX does NOT like sometimes (t5570-git-daemon test 4) to rearm it.\n \t */\n+#ifndef _AIX\n \tsignal(SIGCHLD, child_handler);\n+#endif\n }\n \n static int set_reuse_addr(int sockfd)\n@@ -1155,7 +1158,19 @@\n \t\tpfd[i].events = POLLIN;\n \t}\n \n+#ifdef _AIX\n+\t/* AIX does NOT like sometimes (t5570-git-daemon test 4) to rearm the SIGCHLD handler */\n+\tstruct sigaction sa;\n+\n+\tbzero(&sa, sizeof(sa));\n+\tsa.sa_handler = child_handler;\n+\tsa.sa_flags   = 0;\n+\tsigemptyset(&sa.sa_mask);\n+\n+\tsigaction(SIGCHLD, &sa, NULL);\n+#else\n \tsignal(SIGCHLD, child_handler);\n+#endif\n \n \tfor (;;) {\n \t\tint i;\n"},{"id":"374266","messageId":"20190422195226.GB22798@sigill.intra.peff.net","threadId":"50777","inReplyTo":"DB7PR02MB46634F68900E0F81B0110AF586400@DB7PR02MB4663.eurprd02.prod.outlook.com","subject":"Re: git on AIX: daemon.c & t5570-git-daemon.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-22T19:52:26Z","receivedAt":"2019-04-22T19:52:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2019 at 09:05:49AM +0000, REIX, Tony wrote:\n\n> When testing version 2.21.0 of git on AIX (6.1 & 7.2), I have found an\n> issue with daemon.c and test t5570-git-daemon.sh : within test 4, the\n> child_handler() code gets crazy and calls itself recursively till the\n> process crashes. We do not have a clear idea why this issue occurs.\n> Maybe that this issue also appears on other operating  systems.\n\nInteresting. I think re-arming a signal() has been a well-understood\ntechnique for a while, but I am not too surprised that there would be a\nplatform that doesn't like it. :)\n\nSo I understand this hunk:\n\n> --- ./daemon.c.ORIGIN\t2019-03-18 17:53:51 +0100\n> +++ ./daemon.c\t2019-03-18 18:00:16 +0100\n> @@ -943,8 +943,11 @@\n>  \t * Otherwise empty handler because systemcalls will get interrupted\n>  \t * upon signal receipt\n>  \t * SysV needs the handler to be rearmed\n> +\t * AIX does NOT like sometimes (t5570-git-daemon test 4) to rearm it.\n>  \t */\n> +#ifndef _AIX\n>  \tsignal(SIGCHLD, child_handler);\n> +#endif\n>  }\n\nAlthough usually we would split this out to a Makefile knob. E.g., call\nit something like AVOID_REARMING_SIGNAL_HANDLERS or something, and then\nput it into config.mak.uname for AIX, so that it's turned on by default\nthere. And then if other platforms need the same, they just need to add\na similar Makefile line (and people can experiment by building with or\nwithout it). See how NO_INITGROUPS is used in daemon.c for prior art.\n\nDid you test on a daemon with this patch that when serving multiple\nclients it continues to handle SIGCHLD properly after the first\ninstance? Specifically, we'd want to make sure that our accept()\ncontinues to be interrupted and we run check_dead_children() promptly.\n\n>  static int set_reuse_addr(int sockfd)\n> @@ -1155,7 +1158,19 @@\n>  \t\tpfd[i].events = POLLIN;\n>  \t}\n>  \n> +#ifdef _AIX\n> +\t/* AIX does NOT like sometimes (t5570-git-daemon test 4) to rearm the SIGCHLD handler */\n> +\tstruct sigaction sa;\n> +\n> +\tbzero(&sa, sizeof(sa));\n> +\tsa.sa_handler = child_handler;\n> +\tsa.sa_flags   = 0;\n> +\tsigemptyset(&sa.sa_mask);\n> +\n> +\tsigaction(SIGCHLD, &sa, NULL);\n> +#else\n>  \tsignal(SIGCHLD, child_handler);\n> +#endif\n\nThis hunk I don't quite understand. This signal() should be run only\nonce, so does it matter if it's done via sigaction() or signal()? Does\nAIX require using sigaction() without SA_RESETHAND in order to not need\nthe re-arming behavior?\n\n-Peff\n"}]}