Possible Solaris problem in 'checkout_entry()'

I was looking at this due to the CE_UPDATE bug, and notice that we do

	if (!lstat(path, &st)) {

		... check if it's unchanged ..

		unlink(path);
		if (S_ISDIR(st.st_mode)) {
			..

and it hit me that didn't we have issues with Solaris allowing an "unlink()" to succeed on a directory when you are root, causing various problems later with lost inodes during fsck?

We fixed that in commit fa2e71c9e794c43634670b62d1b4bf58d1ae7e60 back last July, by avoiding to do the unlink() if it was already a directory in create_directories(). But it *looks* like the same problem exists if you use "git checkout -f" and have a directory where you expect a file.

I don't have any access to a Solaris box, nor do I want any, but this test-script (as root, remember) should show if this is a problem:

	mkdir repo
	cd repo
	git init
	echo "Testfile" > a
	git add a
	git commit -m "Initial commit"
	rm a
	mkdir a
	git checkout -f

where you probably need to then reboot and force a fsck to actually see if it caused problems.

Solaris is just totally incredible crap here, but maybe we should move the unlink to after that "if (S_ISDIR(..))" statement? And maybe somebody who has a Solaris support contract can try to kick some Sun *ss to get them to fix their crap?

			Linus

And checking the result from unlink might not hurt either.

Morten

On Mon, 17 Mar 2008, Morten Welinder wrote:

Well, that part is actually intentional. We simply don't care. If the unlink succeeds, we're happy, if it fails, we're happy. No reason to test, really.

(Well, it's not that we're "happy" if the unlink fails, but we actually _expect_ it to fail for directories, and regardless of that we're really doing the _real_ error handling later when we actually create the new entry that will replace the old one, so we don't much care at unlink time).

IOW, the real "checking" is taking place in "create_file()", so if the unlinking failed (due to a read-only directory or something), that's where we'll do the proper error reporting.

		Linus

On Mon, 17 Mar 2008, Linus Torvalds wrote:

Thinking about this, I'm probably full of sh*t.

My argument is admittedly true in general, but there is one case it is *not* true for: if the old entry was a symlink.

IOW, let's imagine that the directory is read-only (or other permission issue), and we want to unlink the old symlink, which points somewhere we can write to. In that case, the symlink removal is important, because we won't necessarily catch the error when we create the file in place later (because that will just follow the symlink).

So I retract my statement. We *should* check the result of the unlink.

So maybe something like this (which does the "avoid Solaris-is-crap" issue too by moving the unlink to being after the directory test).

Untested.

		Linus

---
 entry.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Linus Torvalds <torvalds@linux-foundation.org> writes:

While I agree we should check the result, I think we are safe against the un-unlinkable symlink case. If you have a stale symlink at "dir/file" where you are checking out a new blob, and the directory "dir" the symlink is in is unwritable, then our callpath would look like this:

	checkout_entry()
         unlink("dir/file") -- failure silently ignored which is bad
	 write_entry()
          create_file("dir/file")
           open("dir/file", O_WRONLY | O_CREAT | O_EXCL)

which would fail, and we get:

    error: git-checkout-index: unable to create file a/b (File exists)

from around ll.135 in entry.c::write_entry()

So I'll apply the patch purely as "Root on Solaris safety fix".