{"thread":{"id":"9064","subject":"[PATCH] Do _not_ call unlink on a directory","startedAt":"2007-07-16T17:12:52Z","lastAt":"2007-07-18T08:50:15Z","messageCount":30,"participants":["Thomas Glanzmann","Matthieu Moy","Jan-Benedict Glaw","Brian Downing","Scott Lamb","Linus Torvalds","Junio C Hamano","David Kastrup","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"47563","messageId":"11846059721204-git-send-email-sithglan@stud.uni-erlangen.de","threadId":"9064","inReplyTo":null,"subject":"[PATCH] Do _not_ call unlink on a directory","fromName":"Thomas Glanzmann","fromEmail":"sithglan@stud.uni-erlangen.de","sentAt":"2007-07-16T17:12:52Z","receivedAt":"2007-07-16T17:12:52Z","isPatch":true,"sender":{"key":"sithglan@stud.uni-erlangen.de","avatar":null},"body":"Calling unlink on a directory on a Solaris UFS filesystem as root makes it\ninconsistent. Thanks to Johannes Sixt for the obvious fix.\n---\n entry.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 82bf725..1f2e34d 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -14,10 +14,10 @@ static void create_directories(const char *path, const struct checkout *state)\n \t\tif (mkdir(buf, 0777)) {\n \t\t\tif (errno == EEXIST) {\n \t\t\t\tstruct stat st;\n-\t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\n-\t\t\t\t\tcontinue;\n \t\t\t\tif (!stat(buf, &st) && S_ISDIR(st.st_mode))\n \t\t\t\t\tcontinue; /* ok */\n+\t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\n+\t\t\t\t\tcontinue;\n \t\t\t}\n \t\t\tdie(\"cannot create directory at %s\", buf);\n \t\t}\n-- \n1.5.2.1\n"},{"id":"47565","messageId":"vpqd4yss1vo.fsf@bauges.imag.fr","threadId":"9064","inReplyTo":"11846059721204-git-send-email-sithglan@stud.uni-erlangen.de","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2007-07-16T17:18:51Z","receivedAt":"2007-07-16T17:18:51Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Thomas Glanzmann <sithglan@stud.uni-erlangen.de> writes:\n\nI believe you still have a race condition if ...\n\n> -\t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\n> -\t\t\t\t\tcontinue;\n\n... buf exists here as a file ...\n\n>  \t\t\t\tif (!stat(buf, &st) && S_ISDIR(st.st_mode))\n>  \t\t\t\t\tcontinue; /* ok */\n\n... and became a directory here.\n\n> +\t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\n> +\t\t\t\t\tcontinue;\n\nBut that's quite unlikely to happen. And I have no fix to propose.\n\n-- \nMatthieu\n"},{"id":"47566","messageId":"11846075213759-git-send-email-sithglan@stud.uni-erlangen.de","threadId":"9064","inReplyTo":"11846059721204-git-send-email-sithglan@stud.uni-erlangen.de","subject":"[PATCH] Do _not_ call unlink on a directory","fromName":"Thomas Glanzmann","fromEmail":"sithglan@stud.uni-erlangen.de","sentAt":"2007-07-16T17:38:41Z","receivedAt":"2007-07-16T17:38:41Z","isPatch":true,"sender":{"key":"sithglan@stud.uni-erlangen.de","avatar":null},"body":"Calling unlink on a directory on a Solaris UFS filesystem as root makes it\ninconsistent. Thanks to Johannes Sixt for the obvious fix.\n\nSigned-off-by: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>\n---\n entry.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 82bf725..907293f 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -6,18 +6,18 @@ static void create_directories(const char *path, const struct checkout *state)\n \tint len = strlen(path);\n \tchar *buf = xmalloc(len + 1);\n \tconst char *slash = path;\n+        struct stat st;\n \n \twhile ((slash = strchr(slash+1, '/')) != NULL) {\n \t\tlen = slash - path;\n \t\tmemcpy(buf, path, len);\n \t\tbuf[len] = 0;\n+                if (!stat(buf, &st) && S_ISDIR(st.st_mode))\n+                        continue; /* ok */\n \t\tif (mkdir(buf, 0777)) {\n \t\t\tif (errno == EEXIST) {\n-\t\t\t\tstruct stat st;\n \t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\n \t\t\t\t\tcontinue;\n-\t\t\t\tif (!stat(buf, &st) && S_ISDIR(st.st_mode))\n-\t\t\t\t\tcontinue; /* ok */\n \t\t\t}\n \t\t\tdie(\"cannot create directory at %s\", buf);\n \t\t}\n-- \n1.5.2.1\n"},{"id":"47567","messageId":"20070716174138.GC22998@lug-owl.de","threadId":"9064","inReplyTo":"11846075213759-git-send-email-sithglan@stud.uni-erlangen.de","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Jan-Benedict Glaw","fromEmail":"jbglaw@lug-owl.de","sentAt":"2007-07-16T17:41:38Z","receivedAt":"2007-07-16T17:41:38Z","isPatch":true,"sender":{"key":"jbglaw@lug-owl.de","avatar":null},"body":"On Mon, 2007-07-16 19:38:41 +0200, Thomas Glanzmann <sithglan@stud.uni-erlangen.de> wrote:\n> Calling unlink on a directory on a Solaris UFS filesystem as root makes it\n> inconsistent. Thanks to Johannes Sixt for the obvious fix.\n> \n> Signed-off-by: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>\n> ---\n>  entry.c |    6 +++---\n>  1 files changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/entry.c b/entry.c\n> index 82bf725..907293f 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -6,18 +6,18 @@ static void create_directories(const char *path, const struct checkout *state)\n>  \tint len = strlen(path);\n>  \tchar *buf = xmalloc(len + 1);\n>  \tconst char *slash = path;\n> +        struct stat st;\n\nWhitespace damage.\n\n>  \twhile ((slash = strchr(slash+1, '/')) != NULL) {\n>  \t\tlen = slash - path;\n>  \t\tmemcpy(buf, path, len);\n>  \t\tbuf[len] = 0;\n> +                if (!stat(buf, &st) && S_ISDIR(st.st_mode))\n> +                        continue; /* ok */\n\nDito.\n\n>  \t\tif (mkdir(buf, 0777)) {\n>  \t\t\tif (errno == EEXIST) {\n> -\t\t\t\tstruct stat st;\n>  \t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\n>  \t\t\t\t\tcontinue;\n> -\t\t\t\tif (!stat(buf, &st) && S_ISDIR(st.st_mode))\n> -\t\t\t\t\tcontinue; /* ok */\n>  \t\t\t}\n>  \t\t\tdie(\"cannot create directory at %s\", buf);\n>  \t\t}\n\nMfG, JBG\n\n-- \n      Jan-Benedict Glaw      jbglaw@lug-owl.de              +49-172-7608481\nSignature of:         \"really soon now\":      an unspecified period of time, likly to\nthe second  :                                 be greater than any reasonable definition\n                                              of \"soon\".\n"},{"id":"47568","messageId":"20070716174239.GG19073@lavos.net","threadId":"9064","inReplyTo":"11846075213759-git-send-email-sithglan@stud.uni-erlangen.de","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2007-07-16T17:42:39Z","receivedAt":"2007-07-16T17:42:39Z","isPatch":true,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"On Mon, Jul 16, 2007 at 07:38:41PM +0200, Thomas Glanzmann wrote:\n>  \tconst char *slash = path;\n> +        struct stat st;\n\n>  \t\tmemcpy(buf, path, len);\n>  \t\tbuf[len] = 0;\n> +                if (!stat(buf, &st) && S_ISDIR(st.st_mode))\n> +                        continue; /* ok */\n\nYou've got some whitespace damage here.  Git's style is to use tabs.\n\n-bcd\n"},{"id":"47572","messageId":"20070716175506.GE16780@cip.informatik.uni-erlangen.de","threadId":"9064","inReplyTo":"20070716174239.GG19073@lavos.net","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Thomas Glanzmann","fromEmail":"thomas@glanzmann.de","sentAt":"2007-07-16T17:55:06Z","receivedAt":"2007-07-16T17:55:06Z","isPatch":true,"sender":{"key":"thomas@glanzmann.de","avatar":null},"body":"Hallo Brian,\n\n> You've got some whitespace damage here.  Git's style is to use tabs.\n\nI have expandtab in my vim config which writes 8 spaces instead of tabs.\nI should change that for git.\n\n        Thomas\n"},{"id":"47579","messageId":"469BC17D.60806@slamb.org","threadId":"9064","inReplyTo":"vpqd4yss1vo.fsf@bauges.imag.fr","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Scott Lamb","fromEmail":"slamb@slamb.org","sentAt":"2007-07-16T19:05:33Z","receivedAt":"2007-07-16T19:05:33Z","isPatch":true,"sender":{"key":"slamb@slamb.org","avatar":null},"body":"Matthieu Moy wrote:\n> Thomas Glanzmann <sithglan@stud.uni-erlangen.de> writes:\n> \n> I believe you still have a race condition if ...\n> \n>> -\t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\n>> -\t\t\t\t\tcontinue;\n> \n> ... buf exists here as a file ...\n> \n>>  \t\t\t\tif (!stat(buf, &st) && S_ISDIR(st.st_mode))\n>>  \t\t\t\t\tcontinue; /* ok */\n> \n> ... and became a directory here.\n> \n>> +\t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\n>> +\t\t\t\t\tcontinue;\n> \n> But that's quite unlikely to happen. And I have no fix to propose.\n> \n\nIf arbitrary other tasks are running, the only way to be absolutely\ncertain you're not calling unlink() in a directory is to never call\nunlink().\n\nSUS describes a safe remove(), but Solaris's implementation contains the\nsame race:\n\nhttp://src.opensolaris.org/source/xref/pef/phase_I/usr/src/lib/libc/port/gen/rename.c\n\nso I think this patch is the best that can be done.\n\nBest regards,\nScott\n\n-- \nScott Lamb <http://www.slamb.org/>\n"},{"id":"47581","messageId":"20070716195600.GC16878@cip.informatik.uni-erlangen.de","threadId":"9064","inReplyTo":"469BC17D.60806@slamb.org","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Thomas Glanzmann","fromEmail":"thomas@glanzmann.de","sentAt":"2007-07-16T19:56:00Z","receivedAt":"2007-07-16T19:56:00Z","isPatch":true,"sender":{"key":"thomas@glanzmann.de","avatar":null},"body":"Hello,\n\n> If arbitrary other tasks are running, the only way to be absolutely\n> certain you're not calling unlink() in a directory is to never call\n> unlink().\n\nthere is one way to do it safe but it is so ugly that it is\nunacceptable: don't call unlink as a privileged user (eg. root). So I\nhope that one of the patches make it into git soon. I like the second\npatch better because it does less system calls. Not that it matters.\nFor my co-workers I already build a git version with the patch in so\nthat they can continue to work as root. Don't even think about asking.\n\n        Thomas\n"},{"id":"47583","messageId":"alpine.LFD.0.999.0707161252330.20061@woody.linux-foundation.org","threadId":"9064","inReplyTo":"11846059721204-git-send-email-sithglan@stud.uni-erlangen.de","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-16T19:58:14Z","receivedAt":"2007-07-16T19:58:14Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 16 Jul 2007, Thomas Glanzmann wrote:\n>\n> Calling unlink on a directory on a Solaris UFS filesystem as root makes it\n> inconsistent. Thanks to Johannes Sixt for the obvious fix.\n\nAck, I think this is the right thing to do.\n\nAs pointed out, it doesn't _guarantee_ that git won't call \"unlink()\" on a \ndirectory (race conditions etc), but that's fundamentally true (there is \nno \"funlink()\" like there is \"fstat()\"), and besides, that is in no way \ngit-specific (ie it's true of *any* application that gets run as root).\n\nThe theoretical race would only happen if somebody on purpose tries to \nscrew things over, it would never happen under any reasonable usage. \n\nThe old ordering of those tests was designed for sane operating systems, \nso that you could basically do the unlink() without bothering, but \nswitching the order around is certainly not a disaster either, and if it \navoids the nasty bug in Solaris it's worth doing.\n\nI have to say that I'm still a bit shocked that Solaris would have that \nkind of behaviour. And they call that pile of sh*t \"enterprise class\"..\n\n\t\tLinus\n"},{"id":"47584","messageId":"20070716200024.GD16878@cip.informatik.uni-erlangen.de","threadId":"9064","inReplyTo":"469BC17D.60806@slamb.org","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Thomas Glanzmann","fromEmail":"thomas@glanzmann.de","sentAt":"2007-07-16T20:00:25Z","receivedAt":"2007-07-16T20:00:25Z","isPatch":true,"sender":{"key":"thomas@glanzmann.de","avatar":null},"body":"Hello,\n\n> so I think this patch is the best that can be done.\n\nis there a reason why we call unlink and not remove?\n\n\tThomas\n"},{"id":"47587","messageId":"alpine.LFD.0.999.0707161315120.20061@woody.linux-foundation.org","threadId":"9064","inReplyTo":"20070716200024.GD16878@cip.informatik.uni-erlangen.de","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-16T20:21:47Z","receivedAt":"2007-07-16T20:21:47Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 16 Jul 2007, Thomas Glanzmann wrote:\n> \n> > so I think this patch is the best that can be done.\n> \n> is there a reason why we call unlink and not remove?\n\nExactly because we only want to remove _files_.\n\nIf it's already a directory, we don't need to do anything at all (we just \nwant to go to the next path component).\n\nSo what git wants is the modern \"unlink()\" behaviour that will return \nEPERM (oe EISDIR) for a directory. \n\nNot doing that in this day and age is *insane*. That whole \"unlink/link\" \non directories is original UNIX, but it's original UNIX from several \ndecades ago. It got fixed long long ago, and mkdir/rmdir have existed as \nsystem calls since at least SVR3. Nobody does the insane \"unlink()\" any \nmore.\n\nExcept in Solaris, it would appear.\n\n\t\tLinus\n"},{"id":"47588","messageId":"20070716202550.GH16878@cip.informatik.uni-erlangen.de","threadId":"9064","inReplyTo":"alpine.LFD.0.999.0707161315120.20061@woody.linux-foundation.org","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Thomas Glanzmann","fromEmail":"thomas@glanzmann.de","sentAt":"2007-07-16T20:25:50Z","receivedAt":"2007-07-16T20:25:50Z","isPatch":true,"sender":{"key":"thomas@glanzmann.de","avatar":null},"body":"Hello,\n\n> > is there a reason why we call unlink and not remove?\n\n> Exactly because we only want to remove _files_.\n\nof course. That is the whole point. Call unlink for files, rmdir for\ndirectories.\n\n\tThomas\n"},{"id":"47589","messageId":"alpine.LFD.0.999.0707161327120.20061@woody.linux-foundation.org","threadId":"9064","inReplyTo":"alpine.LFD.0.999.0707161315120.20061@woody.linux-foundation.org","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-16T20:29:38Z","receivedAt":"2007-07-16T20:29:38Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 16 Jul 2007, Linus Torvalds wrote:\n>\n> [..] mkdir/rmdir have existed as \n> system calls since at least SVR3.\n\nCorrection. Apparently since 4.2BSD (1983).\n\n\t\tLinus\n"},{"id":"47590","messageId":"alpine.LFD.0.999.0707161332280.20061@woody.linux-foundation.org","threadId":"9064","inReplyTo":"20070716202550.GH16878@cip.informatik.uni-erlangen.de","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-16T20:34:48Z","receivedAt":"2007-07-16T20:34:48Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 16 Jul 2007, Thomas Glanzmann wrote:\n>\n> Hello,\n> \n> > > is there a reason why we call unlink and not remove?\n> \n> > Exactly because we only want to remove _files_.\n> \n> of course. That is the whole point. Call unlink for files, rmdir for\n> directories.\n\nNo, but we don't *want* the \"rmdir for directories\" part! \n\nThat's the whole point.\n\nCalling \"remove()\" would be *wrong*. We want the *sane* \"unlink()\" \nbehaviour, where it only removes files, and returns an error for \ndirectories.\n\n\t\tLinus\n"},{"id":"47591","messageId":"20070716203950.GI16878@cip.informatik.uni-erlangen.de","threadId":"9064","inReplyTo":"alpine.LFD.0.999.0707161332280.20061@woody.linux-foundation.org","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Thomas Glanzmann","fromEmail":"thomas@glanzmann.de","sentAt":"2007-07-16T20:39:50Z","receivedAt":"2007-07-16T20:39:50Z","isPatch":true,"sender":{"key":"thomas@glanzmann.de","avatar":null},"body":"Hello,\n\n> No, but we don't *want* the \"rmdir for directories\" part! \n\nthat is what I meant. We call unlink because we want unlink to _fail_ on\ndirectories while it deletes file. I forgot about the original\ndiscussion but Johannes refreshed my memory.  If a file in our history\nbecomes a directory we want to get it out of our way. And we want to do\nthat by call unlink.\n\n\tThomas\n"},{"id":"47593","messageId":"20070716210612.GI19073@lavos.net","threadId":"9064","inReplyTo":"alpine.LFD.0.999.0707161252330.20061@woody.linux-foundation.org","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2007-07-16T21:06:12Z","receivedAt":"2007-07-16T21:06:12Z","isPatch":true,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"On Mon, Jul 16, 2007 at 12:58:14PM -0700, Linus Torvalds wrote:\n> I have to say that I'm still a bit shocked that Solaris would have that \n> kind of behaviour. And they call that pile of sh*t \"enterprise class\"..\n\nApparently \"enterprise class\" in this case really means \"fully\ncompatibility with all those wonderful userland implementations of\nrmdir.\"  :-)\n\n-bcd\n"},{"id":"47596","messageId":"alpine.LFD.0.999.0707161414360.20061@woody.linux-foundation.org","threadId":"9064","inReplyTo":"20070716210612.GI19073@lavos.net","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-16T21:19:38Z","receivedAt":"2007-07-16T21:19:38Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 16 Jul 2007, Brian Downing wrote:\n> \n> Apparently \"enterprise class\" in this case really means \"fully\n> compatibility with all those wonderful userland implementations of\n> rmdir.\"  :-)\n\nI see the smiley, but it's actually not possible even for that.\n\nThe broken \"unlink()\" behaviour doesn't work on any other filesystem (eg \nNFS) at all anyway, and even on UFS would only work for root (or \nsetuid-root) binaries. So any user-land that depended on it literally \nwouldn't work _anyway_, even on Solaris itself.\n\nAnd we're talking about the same company that was *famous* for screwing \npeople over when they converted from SunOS to Solaris and broke binaries \n_and_ source code in the process.\n\nSo no, \"compatibility\" can't realistically be the reason.\n\n\t\t\tLinus\n"},{"id":"47597","messageId":"469BE1D4.1070408@slamb.org","threadId":"9064","inReplyTo":"alpine.LFD.0.999.0707161332280.20061@woody.linux-foundation.org","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Scott Lamb","fromEmail":"slamb@slamb.org","sentAt":"2007-07-16T21:23:32Z","receivedAt":"2007-07-16T21:23:32Z","isPatch":true,"sender":{"key":"slamb@slamb.org","avatar":null},"body":"Linus Torvalds wrote:\n> No, but we don't *want* the \"rmdir for directories\" part! \n> \n> That's the whole point.\n> \n> Calling \"remove()\" would be *wrong*. We want the *sane* \"unlink()\" \n> behaviour, where it only removes files, and returns an error for \n> directories.\n\nOf course, but when used immediately after stat() says the path does not\nrefer to a directory, I would prefer SUS remove() (rmdir() for\ndirectories) to Solaris unlink() (break_filesystem() on directories).\n\nBut Solaris remove() is broken, too, so it's a moot point. The\npost-patch behavior is good enough - as you said, it won't happen during\nreasonable usage and the problem's not unique to git.\n\nBest regards,\nScott\n\n-- \nScott Lamb <http://www.slamb.org/>\n"},{"id":"47598","messageId":"alpine.LFD.0.999.0707161442410.20061@woody.linux-foundation.org","threadId":"9064","inReplyTo":"469BE1D4.1070408@slamb.org","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-16T21:44:59Z","receivedAt":"2007-07-16T21:44:59Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 16 Jul 2007, Scott Lamb wrote:\n> \n> But Solaris remove() is broken, too, so it's a moot point.\n\nIn fact, with the Solaris behaviour for unlink(), you *cannot* have a \nnon-broken \"remove()\".\n\nSo the right fix is always to fix \"unlink()\" instead.\n\nThere really aren't any downsides (since no program can rely on it \n_anyway_, unless we're talking about some magic \"early bootup\" time \nscripts that depend on only running as root, and only ever running on UFS \n- but those kinds of scripts could be trivially fixed and are obviously \nunder Sun control anyway)\n\n\t\t\tLinus\n"},{"id":"47600","messageId":"469BEA21.5080308@slamb.org","threadId":"9064","inReplyTo":"alpine.LFD.0.999.0707161442410.20061@woody.linux-foundation.org","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Scott Lamb","fromEmail":"slamb@slamb.org","sentAt":"2007-07-16T21:58:57Z","receivedAt":"2007-07-16T21:58:57Z","isPatch":true,"sender":{"key":"slamb@slamb.org","avatar":null},"body":"Linus Torvalds wrote:\n> \n> On Mon, 16 Jul 2007, Scott Lamb wrote:\n>> But Solaris remove() is broken, too, so it's a moot point.\n> \n> In fact, with the Solaris behaviour for unlink(), you *cannot* have a \n> non-broken \"remove()\".\n\nI'd hoped to see that they made a new syscall to properly implement the\nnew behavior. But they didn't. It reminds me of glibc's pselect().\n\nBest regards,\nScott\n\n-- \nScott Lamb <http://www.slamb.org/>\n"},{"id":"47601","messageId":"alpine.LFD.0.999.0707161502160.20061@woody.linux-foundation.org","threadId":"9064","inReplyTo":"469BEA21.5080308@slamb.org","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-16T22:03:49Z","receivedAt":"2007-07-16T22:03:49Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 16 Jul 2007, Scott Lamb wrote:\n> Linus Torvalds wrote:\n> > \n> > In fact, with the Solaris behaviour for unlink(), you *cannot* have a \n> > non-broken \"remove()\".\n> \n> I'd hoped to see that they made a new syscall to properly implement the\n> new behavior.\n\nAhh, yes, with a new system call you could do it.\n\n> But they didn't. It reminds me of glibc's pselect().\n\nYeah, that was a bit pointless, although it does make it easier to port \nbinaries and then have them to work in practice most of the time.\n\n\t\tLinus\n"},{"id":"47616","messageId":"7v4pk3bi7y.fsf@assigned-by-dhcp.cox.net","threadId":"9064","inReplyTo":"11846059721204-git-send-email-sithglan@stud.uni-erlangen.de","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-17T07:30:09Z","receivedAt":"2007-07-17T07:30:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"},{"id":"47621","messageId":"7vtzs3a0xg.fsf@assigned-by-dhcp.cox.net","threadId":"9064","inReplyTo":"11846059721204-git-send-email-sithglan@stud.uni-erlangen.de","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-17T08:28:59Z","receivedAt":"2007-07-17T08:28:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Glanzmann <sithglan@stud.uni-erlangen.de> writes:\n\n> Calling unlink on a directory on a Solaris UFS filesystem as root makes it\n> inconsistent. Thanks to Johannes Sixt for the obvious fix.\n> ---\n>  entry.c |    4 ++--\n>  1 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/entry.c b/entry.c\n> index 82bf725..1f2e34d 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -14,10 +14,10 @@ static void create_directories(const char *path, const struct checkout *state)\n>  \t\tif (mkdir(buf, 0777)) {\n>  \t\t\tif (errno == EEXIST) {\n>  \t\t\t\tstruct stat st;\n> -\t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\n> -\t\t\t\t\tcontinue;\n>  \t\t\t\tif (!stat(buf, &st) && S_ISDIR(st.st_mode))\n>  \t\t\t\t\tcontinue; /* ok */\n> +\t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\n> +\t\t\t\t\tcontinue;\n>  \t\t\t}\n>  \t\t\tdie(\"cannot create directory at %s\", buf);\n>  \t\t}\n> -- \n> 1.5.2.1\n\nThis is wrong.  If the filesystem has a symlink and we would\nwant a directory there, we should unlink().  So at least the\nstat there needs to be lstat().\n\nI wonder if anybody involved in the discussion has actually\ntested this patch (or the other one, that has the same problem)?\n\nDoes the following replacement work for you?  It adds far more\nlines than your version, but they are mostly comments to make it\nclear why we do things this way.\n\n---\n entry.c |   37 ++++++++++++++++++++++++++++++-------\n 1 files changed, 30 insertions(+), 7 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex f9e7dc5..42fda36 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -8,17 +8,40 @@ static void create_directories(const char *path, const struct checkout *state)\n \tconst char *slash = path;\n \n \twhile ((slash = strchr(slash+1, '/')) != NULL) {\n+\t\tstruct stat st;\n+\t\tint stat_status;\n+\n \t\tlen = slash - path;\n \t\tmemcpy(buf, path, len);\n \t\tbuf[len] = 0;\n+\n+\t\tif (len <= state->base_dir_len)\n+\t\t\t/*\n+\t\t\t * checkout-index --prefix=<dir>; <dir> is\n+\t\t\t * allowed to be a symlink to an existing\n+\t\t\t * directory.\n+\t\t\t */\n+\t\t\tstat_status = stat(buf, &st);\n+\t\telse\n+\t\t\t/*\n+\t\t\t * if there currently is a symlink, we would\n+\t\t\t * want to replace it with a real directory.\n+\t\t\t */\n+\t\t\tstat_status = lstat(buf, &st);\n+\n+\t\tif (!stat_status && S_ISDIR(st.st_mode))\n+\t\t\tcontinue; /* ok, it is already a directory. */\n+\t\t\n+\t\t/*\n+\t\t * We know stat_status == 0 means something exists\n+\t\t * there and this mkdir would fail, but that is an\n+\t\t * error codepath; we do not care, as we unlink and\n+\t\t * mkdir again in such a case.\n+\t\t */\n \t\tif (mkdir(buf, 0777)) {\n-\t\t\tif (errno == EEXIST) {\n-\t\t\t\tstruct stat st;\n-\t\t\t\tif (!stat(buf, &st) && S_ISDIR(st.st_mode))\n-\t\t\t\t\tcontinue; /* ok */\n-\t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\n-\t\t\t\t\tcontinue;\n-\t\t\t}\n+\t\t\tif (errno == EEXIST && state->force &&\n+\t\t\t    !unlink(buf) && !mkdir(buf, 0777))\n+\t\t\t\tcontinue;\n \t\t\tdie(\"cannot create directory at %s\", buf);\n \t\t}\n \t}\n"},{"id":"47623","messageId":"86vecj1k65.fsf@lola.quinscape.zz","threadId":"9064","inReplyTo":"alpine.LFD.0.999.0707161252330.20061@woody.linux-foundation.org","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-07-17T08:58:10Z","receivedAt":"2007-07-17T08:58:10Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Mon, 16 Jul 2007, Thomas Glanzmann wrote:\n>>\n>> Calling unlink on a directory on a Solaris UFS filesystem as root makes it\n>> inconsistent. Thanks to Johannes Sixt for the obvious fix.\n>\n> Ack, I think this is the right thing to do.\n>\n> As pointed out, it doesn't _guarantee_ that git won't call\n> \"unlink()\" on a directory (race conditions etc), but that's\n> fundamentally true (there is no \"funlink()\" like there is\n> \"fstat()\"), and besides, that is in no way git-specific (ie it's\n> true of *any* application that gets run as root).\n\nPlease note that doing \"remove\" before \"mkdir\" without checking for\ndirectoriness still offers a race window where one can slip in a new\nnon-directory file.\n\n-- \nDavid Kastrup\n"},{"id":"47625","messageId":"20070717101527.GB7774@cip.informatik.uni-erlangen.de","threadId":"9064","inReplyTo":"7vtzs3a0xg.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Thomas Glanzmann","fromEmail":"thomas@glanzmann.de","sentAt":"2007-07-17T10:15:27Z","receivedAt":"2007-07-17T10:15:27Z","isPatch":true,"sender":{"key":"thomas@glanzmann.de","avatar":null},"body":"Hello Junio,\n\n> This is wrong.  If the filesystem has a symlink and we would want a\n> directory there, we should unlink().  So at least the stat there needs\n> to be lstat().\n\nI see.\n\n> I wonder if anybody involved in the discussion has actually\n> tested this patch (or the other one, that has the same problem)?\n\nI tested it. But I did not test it with symlinks.\n\n> Does the following replacement work for you?  It adds far more lines\n> than your version, but they are mostly comments to make it clear why\n> we do things this way.\n\nYes, it does. Excuse the delay but my build machine is not the fastest.\n\n\t(faui04a) [/var/tmp] git clone ~/work/repositories/public/easix.git test-10\n\tInitialized empty Git repository in /var/tmp/test-10/.git/\n\tremote: Generating pack...\n\tremote: Done counting 317 objects.\n\tremote: Deltifying 317 objects...\n\tremote: te: % (317/317) done: ) done\n\tIndexing 317 objects...\n\tremote: Total 317 (delta 182), reused 278 (delta 157)\n\t100% (317/317) done\n\tResolving 182 deltas...\n\t100% (182/182) done\n\t(faui04a) [/var/tmp] cd test-10\n\t./test-10\n\t(faui04a) [/var/tmp/test-10] git status\n\t# On branch master\n\tnothing to commit (working directory clean)\n\nI rebased your patch on top of current HEAD (as I can access it on\ngit.kernel.org) and removed trailing whitspace from one line (git-apply\ncomplained)\n\n\tThomas\n\n>From 3b60b807007507ce5e1f8490f1469dac5bb95917 Mon Sep 17 00:00:00 2001\nFrom: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>\nDate: Tue, 17 Jul 2007 11:31:07 +0200\nSubject: [PATCH] Do _not_ call unlink on a directory\n\nCalling unlink on a directory on a Solaris UFS filesystem as root makes it\ninconsistent. Thanks to Junio for the not so obvious fix.\n---\n entry.c |   37 ++++++++++++++++++++++++++++++-------\n 1 files changed, 30 insertions(+), 7 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex c540ae1..0625112 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -8,17 +8,40 @@ static void create_directories(const char *path, const struct checkout *state)\n \tconst char *slash = path;\n \n \twhile ((slash = strchr(slash+1, '/')) != NULL) {\n+\t\tstruct stat st;\n+\t\tint stat_status;\n+\n \t\tlen = slash - path;\n \t\tmemcpy(buf, path, len);\n \t\tbuf[len] = 0;\n+\n+\t\tif (len <= state->base_dir_len)\n+\t\t\t/*\n+\t\t\t * checkout-index --prefix=<dir>; <dir> is\n+\t\t\t * allowed to be a symlink to an existing\n+\t\t\t * directory.\n+\t\t\t */\n+\t\t\tstat_status = stat(buf, &st);\n+\t\telse\n+\t\t\t/*\n+\t\t\t * if there currently is a symlink, we would\n+\t\t\t * want to replace it with a real directory.\n+\t\t\t */\n+\t\t\tstat_status = lstat(buf, &st);\n+\n+\t\tif (!stat_status && S_ISDIR(st.st_mode))\n+\t\t\tcontinue; /* ok, it is already a directory. */\n+\n+\t\t/*\n+\t\t * We know stat_status == 0 means something exists\n+\t\t * there and this mkdir would fail, but that is an\n+\t\t * error codepath; we do not care, as we unlink and\n+\t\t * mkdir again in such a case.\n+\t\t */\n \t\tif (mkdir(buf, 0777)) {\n-\t\t\tif (errno == EEXIST) {\n-\t\t\t\tstruct stat st;\n-\t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\n-\t\t\t\t\tcontinue;\n-\t\t\t\tif (!stat(buf, &st) && S_ISDIR(st.st_mode))\n-\t\t\t\t\tcontinue; /* ok */\n-\t\t\t}\n+\t\t\tif (errno == EEXIST && state->force &&\n+\t\t\t    !unlink(buf) && !mkdir(buf, 0777))\n+\t\t\t\tcontinue;\n \t\t\tdie(\"cannot create directory at %s\", buf);\n \t\t}\n \t}\n-- \n1.5.2.1\n"},{"id":"47659","messageId":"7vlkdeang0.fsf@assigned-by-dhcp.cox.net","threadId":"9064","inReplyTo":"20070717101527.GB7774@cip.informatik.uni-erlangen.de","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-17T18:34:55Z","receivedAt":"2007-07-17T18:34:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Glanzmann <thomas@glanzmann.de> writes:\n\n>> I wonder if anybody involved in the discussion has actually\n>> tested this patch (or the other one, that has the same problem)?\n>\n> I tested it. But I did not test it with symlinks.\n>\n>> Does the following replacement work for you?  It adds far more lines\n>> than your version, but they are mostly comments to make it clear why\n>> we do things this way.\n>\n> Yes, it does. Excuse the delay but my build machine is not the fastest.\n>\n> \t(faui04a) [/var/tmp] git clone ~/work/repositories/public/easix.git test-10\n> \tInitialized empty Git repository in /var/tmp/test-10/.git/\n> \tremote: Generating pack...\n> \tremote: Done counting 317 objects.\n> \tremote: Deltifying 317 objects...\n> \tremote: te: % (317/317) done: ) done\n> \tIndexing 317 objects...\n> \tremote: Total 317 (delta 182), reused 278 (delta 157)\n> \t100% (317/317) done\n> \tResolving 182 deltas...\n> \t100% (182/182) done\n> \t(faui04a) [/var/tmp] cd test-10\n> \t./test-10\n> \t(faui04a) [/var/tmp/test-10] git status\n> \t# On branch master\n> \tnothing to commit (working directory clean)\n\nAhhhh, by \"testing\", I meant \"runnnig the testsuite shipped with\nthe source\".  Both of your patches were failing in somewhere in\nt2000 series of tests.\n\n> I rebased your patch on top of current HEAD (as I can access it on\n> git.kernel.org) and removed trailing whitspace from one line (git-apply\n> complained)\n\nI am thinking that this fix should go to 'maint' and merged to\n'master', as it is a grave problem in at least one setup.\n"},{"id":"47661","messageId":"alpine.LFD.0.999.0707171207090.27353@woody.linux-foundation.org","threadId":"9064","inReplyTo":"7vtzs3a0xg.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-17T19:07:25Z","receivedAt":"2007-07-17T19:07:25Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 17 Jul 2007, Junio C Hamano wrote:\n> \n> This is wrong.  If the filesystem has a symlink and we would\n> want a directory there, we should unlink().  So at least the\n> stat there needs to be lstat().\n\nGood catch. Ack.\n\n\t\tLinus\n"},{"id":"47669","messageId":"20070717202754.GB25037@cip.informatik.uni-erlangen.de","threadId":"9064","inReplyTo":"7vlkdeang0.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Thomas Glanzmann","fromEmail":"thomas@glanzmann.de","sentAt":"2007-07-17T20:27:55Z","receivedAt":"2007-07-17T20:27:55Z","isPatch":true,"sender":{"key":"thomas@glanzmann.de","avatar":null},"body":"Hello Junio,\n\n> Ahhhh, by \"testing\", I meant \"runnnig the testsuite shipped with\n> the source\".  Both of your patches were failing in somewhere in\n> t2000 series of tests.\n\nThat was the last time, I am going to submit a patch _without_ running\nthe whole testsuite before. I hate it myself when other people don't do\nthe obvious tests and break something that worked before.\n\n> I am thinking that this fix should go to 'maint' and merged to\n> 'master', as it is a grave problem in at least one setup.\n\nThanks. For packages that I distribute, I fixed it of course by myself.\nAnd to be precise I use git on Solaris a lot by myself but I don't work\nas root so the bug never showed up before and as you can see by the\npastes that I provided to track down the bug I have\n\n\tif [ $UID -eq 0 ]; then\n\t\texport PS1=\"(${PROMPT_RED}\\h${PROMPT_END}) [${PROMPT_BLUE}\\w${PROMPT_END}] \";\n\t\talias bk='echo DO *NOT* RUN BK AS ROOT'\n\t\talias git='echo DO *NOT* RUN GIT AS ROOT'\n\t\talias links='echo DO *NOT* RUN LINKS AS ROOT'\n\t\talias elinks='echo DO *NOT* RUN ELINKS AS ROOT'\n\t...\n\nin my distributed environment. But my coworker who I \"show\" git to work\na lot as root. A very bad habbit that is hard to get rid of. Btw. I\nprepare to setup a automatic build script which I am going to let run\nautomatic on a daily basis so that I catch Solaris compile problems\nearly and report them to you.\n\n\tThomas\n"},{"id":"47708","messageId":"469DC037.A7D69826@eudaptics.com","threadId":"9064","inReplyTo":"20070717202754.GB25037@cip.informatik.uni-erlangen.de","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Johannes Sixt","fromEmail":"j.sixt@eudaptics.com","sentAt":"2007-07-18T07:24:39Z","receivedAt":"2007-07-18T07:24:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Thomas Glanzmann wrote:\n> and as you can see by the\n> pastes that I provided to track down the bug I have\n> \n>         if [ $UID -eq 0 ]; then\n>                 export PS1=\"(${PROMPT_RED}\\h${PROMPT_END}) [${PROMPT_BLUE}\\w${PROMPT_END}] \";\n>                 alias bk='echo DO *NOT* RUN BK AS ROOT'\n>                 alias git='echo DO *NOT* RUN GIT AS ROOT'\n>                 alias links='echo DO *NOT* RUN LINKS AS ROOT'\n>                 alias elinks='echo DO *NOT* RUN ELINKS AS ROOT'\n\nAnd if you have a file NOTES in $pwd, it will tell you:\n\nDO NOTES RUN GIT AS ROOT\n\n;-P\n\n-- Hannes\n"},{"id":"47715","messageId":"20070718085015.GK25037@cip.informatik.uni-erlangen.de","threadId":"9064","inReplyTo":"469DC037.A7D69826@eudaptics.com","subject":"Re: [PATCH] Do _not_ call unlink on a directory","fromName":"Thomas Glanzmann","fromEmail":"thomas@glanzmann.de","sentAt":"2007-07-18T08:50:15Z","receivedAt":"2007-07-18T08:50:15Z","isPatch":true,"sender":{"key":"thomas@glanzmann.de","avatar":null},"body":"Hello,\n\n> >                 alias elinks='echo DO *NOT* RUN ELINKS AS ROOT'\n\n> And if you have a file NOTES in $pwd, it will tell you:\n> DO NOTES RUN GIT AS ROOT\n\nfixed.\n\n\tThomas\n"}]}