Re: renameat

[Jim Meyering]

Eric Blake wrote:
> According to Jim Meyering on 10/1/2009 1:28 PM:
>>> Here's the current state of the series, finally ready for review.  If we
>>> check it in as-is, then coreutils will have everything it needs to ensure
>>> consistent behavior of 'mv -T a b/' on every platform it already supports
>>> except for cygwin 1.5 (which has never been a show-stopper for coreutils
>>
>> I've skimmed through all of that.
>> Considering I spent only 20 minutes, I can't have
>> done it justice, but didn't spot any problems, either.
>
> I've completed these three additional patches to fix the outstanding
> issues, and am now doing a sanity check that my rebasing worked before
> pushing to savannah.  Expect it soon.

Thanks!
A couple of minor suggestions below:

>>From 0eb9ddbf689cb195763848c52507685ebb07834d Mon Sep 17 00:00:00 2001
> From: Eric Blake <address@hidden>
> Date: Thu, 1 Oct 2009 11:57:47 -0600
> Subject: [PATCH 1/3] rename: fix another cygwin 1.5 bug
...

>>From 984235b49cd5dd2f2bcfe26a4988c1c8a2253b97 Mon Sep 17 00:00:00 2001
> From: Eric Blake <address@hidden>
> Date: Thu, 1 Oct 2009 15:31:32 -0600
> Subject: [PATCH 2/3] renameat: fix Solaris bugs
...

>>From 9b631e0590832e7a1739fe053c30c97fa4d1aa77 Mon Sep 17 00:00:00 2001
> From: Eric Blake <address@hidden>
> Date: Thu, 1 Oct 2009 16:46:08 -0600
> Subject: [PATCH 3/3] rename: fix mingw bugs
>
> Copy various workarounds from cygwin 1.5: rename("dir/.","name"),
> rename("dir","file"), rename("dir1","dir2").  Amazingly,
> even though mingw stat() has no way to identify hard linked
> files, and even though rename("hard1","hard2") destroys the
> hard link, the lower-level MoveFileEx does the right thing!
> So the only testsuite relaxation is for rename("dir","dir/sub")
> giving EACCES instead of EINVAL when "dir/sub" does not exist.
>
> * lib/rename.c (rpl_rename) [W32]: Fix trailing slash and
> directory overwrite bugs.
> * tests/test-rename.h (test_rename): Relax test.
...

> diff --git a/lib/rename.c b/lib/rename.c
...
> +  /* Presence of a trailing slash requires directory semantics.  If
> +     the source does not exist, or if the destination cannot be turned
> +     into a directory, give up now.  Otherwise, strip trailing slashes
> +     before calling rename.  There are no symlinks on mingw, so stat
> +     works instead of lstat.  */
> +  src_slash = ISSLASH (src[src_len - 1]);
> +  dst_slash = ISSLASH (dst[dst_len - 1]);
> +  if (stat (src, &src_st))
> +    return -1;
> +  if (stat (dst, &dst_st))
> +    {
> +      if (errno != ENOENT || (!S_ISDIR (src_st.st_mode) && dst_slash))
> +        return -1;
> +      dst_exists = false;
> +    }
> +  else
> +    {
> +      if (S_ISDIR (dst_st.st_mode) != S_ISDIR (src_st.st_mode))
> +     {

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.

> +       errno = S_ISDIR (dst_st.st_mode) ? EISDIR : ENOTDIR;
> +       return -1;
> +     }
> +      dst_exists = true;
> +    }
> +
> +  /* There are no symlinks, so if a file existed with a trailing
> +     slash, it must be a directory, and we don't have to worry about
> +     stripping strip trailing slash.  However, mingw refuses to
> +     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.  */
> +  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;
> +      if (IS_ABSOLUTE_FILE_NAME (src))
> +        {
> +          dst_temp = chdir (dst) ? NULL : getcwd (NULL, 0);
> +          src_temp = chdir (src) ? NULL : getcwd (NULL, 0);
> +        }
> +      else
> +        {
> +          src_temp = chdir (src) ? NULL : getcwd (NULL, 0);
> +          if (!IS_ABSOLUTE_FILE_NAME (dst))
> +            chdir (cwd);

This chdir may fail.

> +          dst_temp = chdir (dst) ? NULL : getcwd (NULL, 0);
> +        }
> +      chdir (cwd);

Same here.  Neither failure may be ignored.

> +      free (cwd);
> +      if (!src_temp || !dst_temp)
> +        {
> +          free (src_temp);
> +          free (dst_temp);
> +          errno = ENOMEM;
> +          return -1;
> +        }
> +      src_len = strlen (src_temp);
> +      if (strncmp (src_temp, dst_temp, src_len) == 0
> +          && (ISSLASH (dst_temp[src_len]) || dst_temp[src_len] == '\0'))
> +        {
> +          error = dst_temp[src_len];
> +          free (src_temp);
> +          free (dst_temp);
> +          if (error)
> +            {
> +              errno = EINVAL;
> +              return -1;
> +            }
> +          return 0;
> +        }
> +      if (rmdir (dst))
> +        {
> +          error = errno;
> +          free (src_temp);
> +          free (dst_temp);
> +          errno = error;
> +          return -1;
> +        }
> +      free (src_temp);
> +      free (dst_temp);
> +    }
...