{"thread":{"id":"12724","subject":"Possible Solaris problem in 'checkout_entry()'","startedAt":"2008-03-17T15:07:14Z","lastAt":"2008-03-19T01:05:24Z","messageCount":5,"participants":["Linus Torvalds","Morten Welinder","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"72289","messageId":"alpine.LFD.1.00.0803170756390.3020@woody.linux-foundation.org","threadId":"12724","inReplyTo":null,"subject":"Possible Solaris problem in 'checkout_entry()'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-03-17T15:07:14Z","receivedAt":"2008-03-17T15:07:14Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nI was looking at this due to the CE_UPDATE bug, and notice that we do\n\n\tif (!lstat(path, &st)) {\n\n\t\t... check if it's unchanged ..\n\n\t\tunlink(path);\n\t\tif (S_ISDIR(st.st_mode)) {\n\t\t\t..\n\nand it hit me that didn't we have issues with Solaris allowing an \n\"unlink()\" to succeed on a directory when you are root, causing various \nproblems later with lost inodes during fsck?\n\nWe fixed that in commit fa2e71c9e794c43634670b62d1b4bf58d1ae7e60 back last \nJuly, by avoiding to do the unlink() if it was already a directory in \ncreate_directories(). But it *looks* like the same problem exists if you \nuse \"git checkout -f\" and have a directory where you expect a file.\n\nI don't have any access to a Solaris box, nor do I want any, but this \ntest-script (as root, remember) should show if this is a problem:\n\n\tmkdir repo\n\tcd repo\n\tgit init\n\techo \"Testfile\" > a\n\tgit add a\n\tgit commit -m \"Initial commit\"\n\trm a\n\tmkdir a\n\tgit checkout -f\n\nwhere you probably need to then reboot and force a fsck to actually see if \nit caused problems.\n\nSolaris is just totally incredible crap here, but maybe we should move the \nunlink to after that \"if (S_ISDIR(..))\" statement? And maybe somebody who \nhas a Solaris support contract can try to kick some Sun *ss to get them to \nfix their crap?\n\n\t\t\tLinus\n"},{"id":"72295","messageId":"118833cc0803170823q1e1e29a9p18b9a41f6975e268@mail.gmail.com","threadId":"12724","inReplyTo":"alpine.LFD.1.00.0803170756390.3020@woody.linux-foundation.org","subject":"Re: Possible Solaris problem in 'checkout_entry()'","fromName":"Morten Welinder","fromEmail":"mwelinder@gmail.com","sentAt":"2008-03-17T15:23:27Z","receivedAt":"2008-03-17T15:23:27Z","isPatch":false,"sender":{"key":"mwelinder@gmail.com","avatar":null},"body":">                 unlink(path);\n\nAnd checking the result from unlink might not hurt either.\n\nMorten\n"},{"id":"72296","messageId":"alpine.LFD.1.00.0803170832280.3020@woody.linux-foundation.org","threadId":"12724","inReplyTo":"118833cc0803170823q1e1e29a9p18b9a41f6975e268@mail.gmail.com","subject":"Re: Possible Solaris problem in 'checkout_entry()'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-03-17T15:37:09Z","receivedAt":"2008-03-17T15:37:09Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 17 Mar 2008, Morten Welinder wrote:\n>\n> >                 unlink(path);\n> \n> And checking the result from unlink might not hurt either.\n\nWell, that part is actually intentional. We simply don't care. If the \nunlink succeeds, we're happy, if it fails, we're happy. No reason to test, \nreally.\n\n(Well, it's not that we're \"happy\" if the unlink fails, but we actually \n_expect_ it to fail for directories, and regardless of that we're really \ndoing the _real_ error handling later when we actually create the new \nentry that will replace the old one, so we don't much care at unlink \ntime).\n\nIOW, the real \"checking\" is taking place in \"create_file()\", so if the \nunlinking failed (due to a read-only directory or something), that's where \nwe'll do the proper error reporting.\n\n\t\tLinus\n"},{"id":"72298","messageId":"alpine.LFD.1.00.0803170850090.3020@woody.linux-foundation.org","threadId":"12724","inReplyTo":"alpine.LFD.1.00.0803170832280.3020@woody.linux-foundation.org","subject":"Re: Possible Solaris problem in 'checkout_entry()'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-03-17T15:56:27Z","receivedAt":"2008-03-17T15:56:27Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 17 Mar 2008, Linus Torvalds wrote:\n>\n> IOW, the real \"checking\" is taking place in \"create_file()\", so if the \n> unlinking failed (due to a read-only directory or something), that's where \n> we'll do the proper error reporting.\n\nThinking about this, I'm probably full of sh*t.\n\nMy argument is admittedly true in general, but there is one case it is \n*not* true for: if the old entry was a symlink.\n\nIOW, let's imagine that the directory is read-only (or other permission \nissue), and we want to unlink the old symlink, which points somewhere we \ncan write to. In that case, the symlink removal is important, because we \nwon't necessarily catch the error when we create the file in place later \n(because that will just follow the symlink).\n\nSo I retract my statement. We *should* check the result of the unlink.\n\nSo maybe something like this (which does the \"avoid Solaris-is-crap\"\nissue too by moving the unlink to being after the directory test).\n\nUntested.\n\n\t\tLinus\n\n---\n entry.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 44f4b89..222aaa3 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -218,7 +218,6 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t\n \t\t * to emulate by hand - much easier to let the system\n \t\t * just do the right thing)\n \t\t */\n-\t\tunlink(path);\n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\t/* If it is a gitlink, leave it alone! */\n \t\t\tif (S_ISGITLINK(ce->ce_mode))\n@@ -226,7 +225,8 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t\n \t\t\tif (!state->force)\n \t\t\t\treturn error(\"%s is a directory\", path);\n \t\t\tremove_subtree(path);\n-\t\t}\n+\t\t} else if (unlink(path))\n+\t\t\treturn error(\"unable to unlink old '%s' (%s)\", path, strerror(errno));\n \t} else if (state->not_new)\n \t\treturn 0;\n \tcreate_directories(path, state);\n"},{"id":"72366","messageId":"7vwsnz5xrf.fsf@gitster.siamese.dyndns.org","threadId":"12724","inReplyTo":"alpine.LFD.1.00.0803170850090.3020@woody.linux-foundation.org","subject":"Re: Possible Solaris problem in 'checkout_entry()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-19T01:05:24Z","receivedAt":"2008-03-19T01:05:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Mon, 17 Mar 2008, Linus Torvalds wrote:\n>>\n>> IOW, the real \"checking\" is taking place in \"create_file()\", so if the \n>> unlinking failed (due to a read-only directory or something), that's where \n>> we'll do the proper error reporting.\n>\n> Thinking about this, I'm probably full of sh*t.\n>\n> My argument is admittedly true in general, but there is one case it is \n> *not* true for: if the old entry was a symlink.\n>\n> IOW, let's imagine that the directory is read-only (or other permission \n> issue), and we want to unlink the old symlink, which points somewhere we \n> can write to. In that case, the symlink removal is important, because we \n> won't necessarily catch the error when we create the file in place later \n> (because that will just follow the symlink).\n>\n> So I retract my statement. We *should* check the result of the unlink.\n\nWhile I agree we should check the result, I think we are safe against the\nun-unlinkable symlink case.  If you have a stale symlink at \"dir/file\"\nwhere you are checking out a new blob, and the directory \"dir\" the symlink\nis in is unwritable, then our callpath would look like this:\n\n\tcheckout_entry()\n         unlink(\"dir/file\") -- failure silently ignored which is bad\n\t write_entry()\n          create_file(\"dir/file\")\n           open(\"dir/file\", O_WRONLY | O_CREAT | O_EXCL)\n\nwhich would fail, and we get:\n\n    error: git-checkout-index: unable to create file a/b (File exists)\n\nfrom around ll.135 in entry.c::write_entry()\n\nSo I'll apply the patch purely as \"Root on Solaris safety fix\".\n"}]}