-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
According to Jim Meyering on 10/2/2009 10:26 AM:
>
> The odd indentation highlights the fact there's a TAB on this line
> and on the three following. If you use spaces, the code will
> render readably regardless of quoting.
I haven't yet quite made the change to my .emacs to globally use
indent-tabs-mode nil, but I did notice that you did it for coreutils, so I
will start doing it to. At any rate, I pushed a cleanup patch for this.
>> + if (dst_exists && S_ISDIR (dst_st.st_mode))
>> + {
>> + char *cwd = getcwd (NULL, 0);
>
> Don't you want to handle getcwd failure here?
> Otherwise, the chdir below dereferences NULL.
>
>> + char *src_temp;
>> + char *dst_temp;
>> + if (chdir (cwd))
>> + return -1;
Good catch. Fixed below.
>> + if (!IS_ABSOLUTE_FILE_NAME (dst))
>> + chdir (cwd);
>
> This chdir may fail.
Indeed. Although it is a highly unlikely race, since we just proved it
succeeded a moment earlier. fchdir had the same bug (since I copied that
implementation when writing rename). So I added an abort if we ever
encounter that situation.
I also documented that a failed rename (such as if the source is
read-only, and causes EROFS) can end up losing the empty destination
directory, as well as prevented one case of that for cygwin (but st_dev is
worthless on mingw). Fortunately, rename("dir","existing-dir") is enough
of a corner case, and the loss of an empty directory is not very bad, that
I won't lose sleep over it.
Fixed as follows:
- --
Don't work too hard, make some time for fun as well!
Eric Blake e...@byu.net
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAkrGQ6QACgkQ84KuGfSFAYCtZQCeImvGDOBPk4+dFn2beQLnpRHS
l5wAoNimZ0wHJqWkp////saZWcbUXHjY
=fM54
-----END PGP SIGNATURE-----
>From 2763d20edded7c1434d58e36c5dd05ed68784f90 Mon Sep 17 00:00:00 2001
From: Eric Blake <e...@byu.net>
Date: Fri, 2 Oct 2009 12:05:02 -0600
Subject: [PATCH] rename, fchdir: don't ignore chdir failure
Although we just checked that chdir(cwd) worked, there is a
race where it could disappear while we are temporarily away.
If that happens, forcefully give up rather than proceeding
in the wrong directory.
* lib/fchdir.c (get_name): Abort on unexpected chdir failure.
* lib/rename.c (rpl_rename) [W32]: Likewise.
(rpl_rename) [RENAME_DEST_EXISTS_BUG]: Avoid one case of losing
an empty destination directory if source cannot be renamed,
although there is still possibility for failure.
* doc/posix-functions/rename.texi (rename): Document the race.
Reported by Jim Meyering.
Signed-off-by: Eric Blake <e...@byu.net>
---
ChangeLog | 9 +++++++++
doc/posix-functions/rename.texi | 4 +++-
lib/fchdir.c | 3 ++-
lib/rename.c | 23 +++++++++++++++++------
4 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index a8e9cf2..43b4bfe 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,14 @@
2009-10-02 Eric Blake <e...@byu.net>
+ rename, fchdir: don't ignore chdir failure
+ * lib/fchdir.c (get_name): Abort on unexpected chdir failure.
+ * lib/rename.c (rpl_rename) [W32]: Likewise.
+ (rpl_rename) [RENAME_DEST_EXISTS_BUG]: Avoid one case of losing
+ an empty destination directory if source cannot be renamed,
+ although there is still possibility for failure.
+ * doc/posix-functions/rename.texi (rename): Document the race.
+ Reported by Jim Meyering.
+
maint: cleanup whitespace in recent commits
* lib/rename.c (rpl_rename): Remove tabs.
* tests/test-link.h (test_link): Likewise.
diff --git a/doc/posix-functions/rename.texi b/doc/posix-functions/rename.texi
index 65981db..be997aa 100644
--- a/doc/posix-functions/rename.texi
+++ b/doc/posix-functions/rename.texi
@@ -27,7 +27,9 @@ rename
@item
This function will not always replace an existing destination on some
platforms:
-mingw.
+Cygwin 1.5.x, mingw.
+However, the replacement is not atomic for directories, and may end up
+losing the empty destination if the source could not be renamed.
@item
This function mistakenly allows names ending in @samp{.} or @samp{..}
on some platforms:
diff --git a/lib/fchdir.c b/lib/fchdir.c
index 54bbc62..e55ef6c 100644
--- a/lib/fchdir.c
+++ b/lib/fchdir.c
@@ -106,7 +106,8 @@ get_name (char const *dir)
return NULL;
result = chdir (dir) ? NULL : getcwd (NULL, 0);
saved_errno = errno;
- chdir (cwd);
+ if (chdir (cwd))
+ abort ();
free (cwd);
errno = saved_errno;
}
diff --git a/lib/rename.c b/lib/rename.c
index 44c9e75..71b8032 100644
--- a/lib/rename.c
+++ b/lib/rename.c
@@ -113,13 +113,16 @@ rpl_rename (char const *src, char const *dst)
replace an existing empty directory, so we have to help it out.
And canonicalize_file_name is not yet ported to mingw; however,
for directories, getcwd works as a viable alternative. Ensure
- that we can get back to where we started before using it. */
+ that we can get back to where we started before using it; later
+ attempts to return are fatal. Note that we can end up losing a
+ directory if rename then fails, but it was empty, so not much
+ damage was done. */
if (dst_exists && S_ISDIR (dst_st.st_mode))
{
char *cwd = getcwd (NULL, 0);
char *src_temp;
char *dst_temp;
- if (chdir (cwd))
+ if (!cwd || chdir (cwd))
return -1;
if (IS_ABSOLUTE_FILE_NAME (src))
{
@@ -129,11 +132,12 @@ rpl_rename (char const *src, char const *dst)
else
{
src_temp = chdir (src) ? NULL : getcwd (NULL, 0);
- if (!IS_ABSOLUTE_FILE_NAME (dst))
- chdir (cwd);
+ if (!IS_ABSOLUTE_FILE_NAME (dst) && chdir (cwd))
+ abort ();
dst_temp = chdir (dst) ? NULL : getcwd (NULL, 0);
}
- chdir (cwd);
+ if (chdir (cwd))
+ abort ();
free (cwd);
if (!src_temp || !dst_temp)
{
@@ -421,9 +425,16 @@ rpl_rename (char const *src, char const *dst)
directory on top of an empty one (the old directory name can
reappear if the new directory tree is removed). Work around this
by removing the target first, but don't remove the target if it
- is a subdirectory of the source. */
+ is a subdirectory of the source. Note that we can end up losing
+ a directory if rename then fails, but it was empty, so not much
+ damage was done. */
if (dst_exists && S_ISDIR (dst_st.st_mode))
{
+ if (src_st.st_dev != dst_st.st_dev)
+ {
+ rename_errno = EXDEV;
+ goto out;
+ }
if (src_temp != src)
free (src_temp);
src_temp = canonicalize_file_name (src);
--
1.6.5.rc1
