Re: [PATCH] Do _not_ call unlink on a directory

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thomas Glanzmann <sithglan@xxxxxxxxxxxxxxxxxxxx> writes:

> Calling unlink on a directory on a Solaris UFS filesystem as root makes it
> inconsistent. Thanks to Johannes Sixt for the obvious fix.
> ---
>  entry.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index 82bf725..1f2e34d 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -14,10 +14,10 @@ static void create_directories(const char *path, const struct checkout *state)
>  		if (mkdir(buf, 0777)) {
>  			if (errno == EEXIST) {
>  				struct stat st;
> -				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
> -					continue;
>  				if (!stat(buf, &st) && S_ISDIR(st.st_mode))
>  					continue; /* ok */
> +				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
> +					continue;
>  			}
>  			die("cannot create directory at %s", buf);
>  		}
> -- 
> 1.5.2.1

This is wrong.  If the filesystem has a symlink and we would
want a directory there, we should unlink().  So at least the
stat there needs to be lstat().

I wonder if anybody involved in the discussion has actually
tested this patch (or the other one, that has the same problem)?

Does the following replacement work for you?  It adds far more
lines than your version, but they are mostly comments to make it
clear why we do things this way.

---
 entry.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/entry.c b/entry.c
index f9e7dc5..42fda36 100644
--- a/entry.c
+++ b/entry.c
@@ -8,17 +8,40 @@ static void create_directories(const char *path, const struct checkout *state)
 	const char *slash = path;
 
 	while ((slash = strchr(slash+1, '/')) != NULL) {
+		struct stat st;
+		int stat_status;
+
 		len = slash - path;
 		memcpy(buf, path, len);
 		buf[len] = 0;
+
+		if (len <= state->base_dir_len)
+			/*
+			 * checkout-index --prefix=<dir>; <dir> is
+			 * allowed to be a symlink to an existing
+			 * directory.
+			 */
+			stat_status = stat(buf, &st);
+		else
+			/*
+			 * if there currently is a symlink, we would
+			 * want to replace it with a real directory.
+			 */
+			stat_status = lstat(buf, &st);
+
+		if (!stat_status && S_ISDIR(st.st_mode))
+			continue; /* ok, it is already a directory. */
+		
+		/*
+		 * We know stat_status == 0 means something exists
+		 * there and this mkdir would fail, but that is an
+		 * error codepath; we do not care, as we unlink and
+		 * mkdir again in such a case.
+		 */
 		if (mkdir(buf, 0777)) {
-			if (errno == EEXIST) {
-				struct stat st;
-				if (!stat(buf, &st) && S_ISDIR(st.st_mode))
-					continue; /* ok */
-				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
-					continue;
-			}
+			if (errno == EEXIST && state->force &&
+			    !unlink(buf) && !mkdir(buf, 0777))
+				continue;
 			die("cannot create directory at %s", buf);
 		}
 	}

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux