bug#10686: mv: moving hardlink of a softlink to the softlink does nothing
Jim Meyering wrote:
> Eric Blake wrote:
>
>> On 02/01/2012 05:47 AM, Jim Meyering wrote:
>>> Bernhard Voelker wrote:
Playing around with the latest mv checkin,
I noticed another corner case:
Create a file 'f', a symlink 'l' to it, and
then a hardlink 's' to that symlink:
$ touch f && ln -s f l && ln l s && ls -ogi
total 0
6444 -rw-r--r-- 1 0 Feb 1 08:52 f
6462 lrwxrwxrwx 2 1 Feb 1 08:52 l -> f
6462 lrwxrwxrwx 2 1 Feb 1 08:52 s -> f
>>>
>>> Hi Bernhard,
>>> Thanks for the report.
>>>
>>> Here, you've already gotten into questionable territory, since the
>>> idea of a hard link to a symbolic link is not standardized.
>>
>> Actually, POSIX 2008 _did_ standardize the notion of a hard link to a
>> symlink, thanks to linkat().
>>
>>> For example, on FreeBSD 9.0, you cannot even create one:
>>
>> That's a POSIX-compliance bug in FreeBSD. In that case, the best we can
>> do is emulate it by creating a new symlink with identical contents and
>> owner and as much of the timestamp as lutimens will allow.
>>
>>>
>>> It's a standards and kernel issue.
>>> POSIX explicitly says (of rename)
>>>
>>> If the old argument and the new argument resolve to the same existing
>>> file, rename( ) shall return successfully and perform no other action.
>>>
>>> though that particular wording might be slated to change with POSIX 2008,
>>> to allow different (more desirable) behavior. Search the austin-group
>>> archives if you need to be sure.
>>
>> The wording for rename(2) did not change, but the wording for mv(1)
>> _did_ change to allow slightly more desirable behavior. Here's the
>> latest wording, as modified by the latest bug report:
>>
>> http://austingroupbugs.net/view.php?id=534
>>
>> If the source_file operand and destination path resolve to either the
>> same existing directory entry or different directory entries for the
>> same existing file, then the destination path shall not be removed, and
>> one of the following shall occur:
>> a. No change is made to source_file, no error occurs, and no diagnostic
>> is issued.
>> b. No change is made to source_file, a diagnostic is issued to standard
>> error identifying the two names, and the exit status is affected.
>> c. If the source_file operand and destination path name distinct
>> directory entries, then the source_file operand is removed, no error
>> occurs, and no diagnostic is issued.
>>
>> The mv utility shall do nothing more with the current source_file, and
>> go on to any remaining source_files.
>>
>>> Also, I don't know whether the above wording applies to this particular
>>> case of two hard links to the same symlink. Again, I think we're in
>>> unspecified territory.
>>
>> POSIX actually specifies this quite well - two hardlinks to the same
>> symlink qualify as an instance of two file names that resolve to the
>> same inode after path resolution as checked with lstat().
>
> Thanks for the clarification and quotes, Eric.
>
> Bernhard, here's a better patch.
> With it, mv simply unlinks the "s" in your example:
>
> diff --git a/src/copy.c b/src/copy.c
> index 4810de8..73f5cc5 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -1229,7 +1229,17 @@ same_file_ok (char const *src_name, struct stat const
> *src_sb,
> know this here IFF preserving symlinks), then it's ok -- as long
> as they are distinct. */
>if (S_ISLNK (src_sb->st_mode) && S_ISLNK (dst_sb->st_mode))
> -return ! same_name (src_name, dst_name);
> +{
> + bool sn = same_name (src_name, dst_name);
> + if ( ! sn && same_link)
> +{
> + *unlink_src = true;
> + *return_now = true;
> + return true;
> +}
> +
> + return ! sn;
> +}
The above wasn't quite right in that it failed to honor mv's --backup
option. mv --backup s f would not have created the required backup file.
I've adjusted it to fix that, and added tests to cover both cases.
This is still not quite ready (i.e., the FIXME comment, where I'll add
some explanation), but otherwise should be fine.
>From 646456b8ce053b85d30174462620492df648e0e2 Mon Sep 17 00:00:00 2001
From: Jim Meyering
Date: Wed, 1 Feb 2012 21:42:45 +0100
Subject: [PATCH] mv: "mv A B" would sometimes succeed, yet A would remain,
...
But only when both A and B were hard links to the same symlink.
* src/copy.c (same_file_ok): Handle another special case: the one
in which we are moving a symlink onto a hard link to itself.
In this case, we must explicitly tell the caller to unlink the
source file. Otherwise, at least the linux-3.x kernel rename
function would do nothing, as mandated by POSIX 2008.
* tests/mv/symlink-onto-hardlink-to-self: New test.
* tests/Makefile.am (TESTS): Add it.
* NEWS (Bug fixes): Mention it.
Reported by Bernhard Voelker in http://bugs.gnu.org/10686
---
NEWS |5 +++
src/copy.c
bug#10472: [PATCH] canonicalize: fix // handling
On Cygwin, and other platforms where // is detected as distinct
from / at configure time, the canonicalize routines were incorrectly
treating all instances of multiple leading slashes as //.
See also coreutils bug http://debbugs.gnu.org/10472
* lib/canonicalize.c (canonicalize_filename_mode): Don't convert
/// to //, since only // is special.
Signed-off-by: Eric Blake
---
ChangeLog |6 ++
lib/canonicalize.c | 12 +++-
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 4bbe447..a9aa40a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2012-02-04 Eric Blake
+
+ canonicalize: fix // handling
+ * lib/canonicalize.c (canonicalize_filename_mode): Don't convert
+ /// to //, since only // is special.
+
2012-02-04 Bruno Haible
ioctl: Fix test failure on native Windows.
diff --git a/lib/canonicalize.c b/lib/canonicalize.c
index d3e5645..ed094b7 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -145,7 +145,7 @@ canonicalize_filename_mode (const char *name,
canonicalize_mode_t can_mode)
rname_limit = rname + PATH_MAX;
rname[0] = '/';
dest = rname + 1;
- if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/')
+ if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] != '/')
*dest++ = '/';
}
@@ -169,7 +169,7 @@ canonicalize_filename_mode (const char *name,
canonicalize_mode_t can_mode)
if (dest > rname + 1)
while ((--dest)[-1] != '/');
if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1
- && *dest == '/')
+ && *dest == '/' && dest[1] != '/')
dest++;
}
else
@@ -267,7 +267,8 @@ canonicalize_filename_mode (const char *name,
canonicalize_mode_t can_mode)
if (buf[0] == '/')
{
dest = rname + 1; /* It's an absolute symlink */
- if (DOUBLE_SLASH_IS_DISTINCT_ROOT && buf[1] == '/')
+ if (DOUBLE_SLASH_IS_DISTINCT_ROOT
+ && buf[1] == '/' && buf[2] != '/')
*dest++ = '/';
}
else
@@ -277,7 +278,7 @@ canonicalize_filename_mode (const char *name,
canonicalize_mode_t can_mode)
if (dest > rname + 1)
while ((--dest)[-1] != '/');
if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1
- && *dest == '/')
+ && *dest == '/' && dest[1] != '/')
dest++;
}
@@ -295,7 +296,8 @@ canonicalize_filename_mode (const char *name,
canonicalize_mode_t can_mode)
}
if (dest > rname + 1 && dest[-1] == '/')
--dest;
- if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1 && *dest == '/')
+ if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1
+ && *dest == '/' && dest[1] != '/')
dest++;
*dest = '\0';
if (rname_limit != dest + 1)
--
1.7.7.6
bug#10472: [PATCH] canonicalize: fix // handling
On 02/04/2012 09:56 AM, Eric Blake wrote: > On Cygwin, and other platforms where // is detected as distinct > from / at configure time, the canonicalize routines were incorrectly > treating all instances of multiple leading slashes as //. > See also coreutils bug http://debbugs.gnu.org/10472 > > * lib/canonicalize.c (canonicalize_filename_mode): Don't convert > /// to //, since only // is special. > > +++ b/lib/canonicalize.c > @@ -145,7 +145,7 @@ canonicalize_filename_mode (const char *name, > canonicalize_mode_t can_mode) >rname_limit = rname + PATH_MAX; >rname[0] = '/'; >dest = rname + 1; > - if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/') > + if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] != '/') > *dest++ = '/'; This never initializes rname[1] if name is "///", > @@ -295,7 +296,8 @@ canonicalize_filename_mode (const char *name, > canonicalize_mode_t can_mode) > } >if (dest > rname + 1 && dest[-1] == '/') > --dest; > - if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1 && *dest == '/') > + if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1 > + && *dest == '/' && dest[1] != '/') > dest++; which meant this was reading uninitialized memory, and depending on what was in the heap, might canonicalize "///" to "/" or "//". I'm pushing this additional fix to both files: diff --git i/lib/canonicalize-lgpl.c w/lib/canonicalize-lgpl.c index a61bef9..08e76fe 100644 --- i/lib/canonicalize-lgpl.c +++ w/lib/canonicalize-lgpl.c @@ -158,6 +158,7 @@ __realpath (const char *name, char *resolved) dest = rpath + 1; if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] != '/') *dest++ = '/'; + *dest = '\0'; } for (start = end = name; *start; start = end) diff --git i/lib/canonicalize.c w/lib/canonicalize.c index ed094b7..2c73094 100644 --- i/lib/canonicalize.c +++ w/lib/canonicalize.c @@ -147,6 +147,7 @@ canonicalize_filename_mode (const char *name, canonicalize_mode_t can_mode) dest = rname + 1; if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] != '/') *dest++ = '/'; + *dest = '\0'; } for (start = name; *start; start = end) -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
bug#10472: [PATCH] canonicalize: fix // handling
On 02/04/2012 10:59 AM, Eric Blake wrote:
> On 02/04/2012 09:56 AM, Eric Blake wrote:
>> On Cygwin, and other platforms where // is detected as distinct
>> from / at configure time, the canonicalize routines were incorrectly
>> treating all instances of multiple leading slashes as //.
>> See also coreutils bug http://debbugs.gnu.org/10472
>>
>> * lib/canonicalize.c (canonicalize_filename_mode): Don't convert
>> /// to //, since only // is special.
>>
>
> which meant this was reading uninitialized memory, and depending on what
> was in the heap, might canonicalize "///" to "/" or "//". I'm pushing
> this additional fix to both files:
>
> diff --git i/lib/canonicalize-lgpl.c w/lib/canonicalize-lgpl.c
> index a61bef9..08e76fe 100644
> --- i/lib/canonicalize-lgpl.c
> +++ w/lib/canonicalize-lgpl.c
> @@ -158,6 +158,7 @@ __realpath (const char *name, char *resolved)
>dest = rpath + 1;
>if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] !=
> '/')
> *dest++ = '/';
> + *dest = '\0';
> }
Still not right. If you have a symlink at //some/path whose contents is
/, then that would canonicalize to '//' without triggering any valgrind
complaints, because I missed the code that resets rpath on encountering
absolute symlink contents. Meanwhile, pre-assigning *dest is a
pessimization on platforms where // and / are identical. I'm pushing
this instead.
From d1f3998942236194f1894c45804ec947d07ed134 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Sat, 4 Feb 2012 11:11:40 -0700
Subject: [PATCH] canonicalize: avoid uninitialized memory use
When DOUBLE_SLASH_IS_DISTINCT_ROOT is non-zero, then we were
reading the contents of rpath[1] even when we had never written
anything there, which meant that "///" would usually canonicalize
to "/" but sometimes to "//" if a '/' was leftover in the heap.
This condition could also occur via 'ln -s / //some/path' and
canonicalizing //some/path, where we rewind rpath but do not
clear out the previous round. Platforms where "//" and "/" are
equivalent do not suffer from this read-beyond-written bounds.
* lib/canonicalize-lgpl.c (__realpath): Avoid possibility of
random '/' left in dest.
* lib/canonicalize.c (canonicalize_filename_mode): Likewise.
Signed-off-by: Eric Blake
---
ChangeLog |7 +++
lib/canonicalize-lgpl.c | 17 -
lib/canonicalize.c | 17 -
3 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 8f08543..aeea7c8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2012-02-04 Eric Blake
+
+ canonicalize: avoid uninitialized memory use
+ * lib/canonicalize-lgpl.c (__realpath): Avoid possibility of
+ random '/' left in dest.
+ * lib/canonicalize.c (canonicalize_filename_mode): Likewise.
+
2012-02-04 Bruno Haible
spawn-pipe tests: Fix a NULL program name in a diagnostic.
diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c
index a61bef9..7aa2d92 100644
--- a/lib/canonicalize-lgpl.c
+++ b/lib/canonicalize-lgpl.c
@@ -156,8 +156,12 @@ __realpath (const char *name, char *resolved)
{
rpath[0] = '/';
dest = rpath + 1;
- if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] !=
'/')
-*dest++ = '/';
+ if (DOUBLE_SLASH_IS_DISTINCT_ROOT)
+{
+ if (name[1] == '/' && name[2] != '/')
+*dest++ = '/';
+ *dest = '\0';
+}
}
for (start = end = name; *start; start = end)
@@ -298,9 +302,12 @@ __realpath (const char *name, char *resolved)
if (buf[0] == '/')
{
dest = rpath + 1; /* It's an absolute symlink */
- if (DOUBLE_SLASH_IS_DISTINCT_ROOT
- && buf[1] == '/' && buf[2] != '/')
-*dest++ = '/';
+ if (DOUBLE_SLASH_IS_DISTINCT_ROOT)
+{
+ if (buf[1] == '/' && buf[2] != '/')
+*dest++ = '/';
+ *dest = '\0';
+}
}
else
{
diff --git a/lib/canonicalize.c b/lib/canonicalize.c
index ed094b7..583c1a4 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -145,8 +145,12 @@ canonicalize_filename_mode (const char *name,
canonicalize_mode_t can_mode)
rname_limit = rname + PATH_MAX;
rname[0] = '/';
dest = rname + 1;
- if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] !=
'/')
-*dest++ = '/';
+ if (DOUBLE_SLASH_IS_DISTINCT_ROOT)
+{
+ if (name[1] == '/' && name[2] != '/')
+*dest++ = '/';
+ *dest = '\0';
+}
}
for (start = name; *start; start = end)
@@ -267,9 +271,12 @@ canonicalize_filename_mode (const char *name,
canonicalize_mode_t can_mode)
if (buf[0] == '/')
{
dest = rname + 1;
bug#10472: [PATCH] realpath: fix problems with // handling
On platforms like Cygwin where / and // are distinct, realpath was
incorrectly collapsing // into /. http://debbugs.gnu.org/10472.
* src/realpath.c (path_common_prefix): When // is special, treat /
and // as having no common match.
(relpath): Allow for no match even without --relative-base.
---
This is the coreutils side of the patch; for this to work, we also
have to upgrade to the latest gnulib.
I'm not pushing this until we decide what to do about testing; I
guess we've already figured out how to make basename and dirname
tests conditional on whether they are on Linux or Cygwin (that is,
whether // and / are the same or different), so I should do something
similar to that for the realpath test. But I can at least get a
code review on realpath.c while figuring out the testing situation.
src/realpath.c |7 +++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/src/realpath.c b/src/realpath.c
index 2dc5e11..f06fbcc 100644
--- a/src/realpath.c
+++ b/src/realpath.c
@@ -131,6 +131,10 @@ path_common_prefix (const char *path1, const char *path2)
int i = 0;
int ret = 0;
+ if (DOUBLE_SLASH_IS_DISTINCT_ROOT && *path1 == '/' && *path2 == '/'
+ && (path1[1] == '/') != (path2[1] == '/'))
+return 0;
+
while (*path1 && *path2)
{
if (*path1 != *path2)
@@ -168,6 +172,9 @@ relpath (const char *can_fname)
/* Skip the prefix common to --relative-to and path. */
int common_index = path_common_prefix (can_relative_to, can_fname);
+ if (!common_index)
+return false;
+
const char *relto_suffix = can_relative_to + common_index;
const char *fname_suffix = can_fname + common_index;
--
1.7.7.6
