bug#10686: mv: moving hardlink of a softlink to the softlink does nothing

2012-02-01 Thread Bernhard Voelker 
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

Trying to mv the hardlink over the symlink seems to succeed:

  $ ~/git/coreutils/src/mv s l

... but the name 's' was not unlinked:

  $ 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

Using the -v option looks also normal, but is a nop:

  $ ~/git/coreutils/src/mv -v s l
  ‘s’ - ‘l’
  $ 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

The strace only shows the rename():

  stat(l, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
  lstat(s, {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
  lstat(l, {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
  rename(s, l)= 0


I'd have expected either
a) the name 's' to be unlinked, or
b) an error message that something was wrong (well, whatever).
I'd prefer a) of course.

That behaviour didn't change at least since version 7.1
(on openSuSE-11.3), but back in the earlier days in 5.93
(SLES-10.3), 's' disappeared as expected:

  stat(l, {st_mode=S_IFREG|0640, st_size=0, ...}) = 0
  lstat(s, {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
  lstat(l, {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
  unlink(l) = 0
  rename(s, l)= 0

Does mv now work as specified? Is this a kernel or
a coreutils (or a user) issue?

Have a nice day,
Berny

bug#10639: BUG REPORT coreutils-8.15 Solaris 10 64bit

2012-02-01 Thread Paul Eggert 
On 01/30/2012 07:01 PM, JONES, BILL wrote:
 those are vxfs.

Thanks, can you please try the attached patch?
You can apply it by running the shell commands:

  cd lib
  patch  vxfs-patch.txt

I'll CC: this to bug-gnulib as it appears to be a Gnulib bug.

Thanks.
acl: fix infinite loop on Solaris 10 + vxfs
Problem reported by Bill Jones in
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10639.
* lib/acl-internal.h: Include limits.h.
(MIN): New macro.
* lib/copy-acl.c (qcopy_acl): Don't object if we get fewer ACL
entries than we originally counted; perhaps some were removed
as we were running, or perhaps we can count but not get.
Check for size-calculation overflow.
Avoid need for dynamic allocation if possible.
* lib/file-has-acl.c (file_has_acl): Likewise.
* lib/set-mode-acl.c (qset_acl): Likewise.
diff --git a/lib/acl-internal.h b/lib/acl-internal.h
index 88e5e45..64701b2 100644
--- a/lib/acl-internal.h
+++ b/lib/acl-internal.h
@@ -55,6 +55,12 @@ extern int aclsort (int, int, struct acl *);
 # define ENOTSUP (-1)
 #endif

+#include limits.h
+
+#ifndef MIN
+# define MIN(a,b) ((a)  (b) ? (a) : (b))
+#endif
+
 #ifndef HAVE_FCHMOD
 # define HAVE_FCHMOD false
 # define fchmod(fd, mode) (-1)
diff --git a/lib/copy-acl.c b/lib/copy-acl.c
index 9b9f033..e9f4826 100644
--- a/lib/copy-acl.c
+++ b/lib/copy-acl.c
@@ -181,10 +181,10 @@ qcopy_acl (const char *src_name, int source_desc, const 
char *dst_name,
  of Unixware.  The acl() call returns the access and default ACL both
  at once.  */
 # ifdef ACE_GETACL
-  int ace_count;
+  int ace_count0, ace_count;
   ace_t *ace_entries;
 # endif
-  int count;
+  int count0, count;
   aclent_t *entries;
   int did_chmod;
   int saved_errno;
@@ -228,19 +228,21 @@ qcopy_acl (const char *src_name, int source_desc, const 
char *dst_name,
   break;
 }

-  ace_entries = (ace_t *) malloc (ace_count * sizeof (ace_t));
-  if (ace_entries == NULL)
+  if (! (ace_count  MIN (INT_MAX, (size_t) -1 / sizeof (ace_t))
+  (ace_entries
+ = (ace_t *) malloc ((ace_count + 1) * sizeof (ace_t)
 {
   errno = ENOMEM;
   return -2;
 }

-  if ((source_desc != -1
-   ? facl (source_desc, ACE_GETACL, ace_count, ace_entries)
-   : acl (src_name, ACE_GETACL, ace_count, ace_entries))
-  == ace_count)
+  ace_count0 = ace_count;
+  ace_count = (source_desc != -1
+   ? facl (source_desc, ACE_GETACL, ace_count + 1, ace_entries)
+   : acl (src_name, ACE_GETACL, ace_count + 1, ace_entries));
+  if (ace_count = ace_count0)
 break;
-  /* Huh? The number of ACL entries changed since the last call.
+  /* Huh? The number of ACL entries grew since the last call.
  Repeat.  */
 }
 # endif
@@ -269,19 +271,21 @@ qcopy_acl (const char *src_name, int source_desc, const 
char *dst_name,
   break;
 }

-  entries = (aclent_t *) malloc (count * sizeof (aclent_t));
-  if (entries == NULL)
+  if (! (count  MIN (INT_MAX, (size_t) -1 / sizeof (aclent_t))
+  (entries
+ = (aclent_t *) malloc ((count + 1) * sizeof (aclent_t)
 {
   errno = ENOMEM;
   return -2;
 }

-  if ((source_desc != -1
-   ? facl (source_desc, GETACL, count, entries)
-   : acl (src_name, GETACL, count, entries))
-  == count)
+  count0 = count;
+  count = (source_desc != -1
+   ? facl (source_desc, GETACL, count + 1, entries)
+   : acl (src_name, GETACL, count + 1, entries));
+  if (count = count0)
 break;
-  /* Huh? The number of ACL entries changed since the last call.
+  /* Huh? The number of ACL entries grew since the last call.
  Repeat.  */
 }

@@ -366,77 +370,44 @@ qcopy_acl (const char *src_name, int source_desc, const 
char *dst_name,
 #elif USE_ACL  HAVE_GETACL /* HP-UX */

   int count;
-  struct acl_entry entries[NACLENTRIES];
+  struct acl_entry entries[NACLENTRIES + 1];
 # if HAVE_ACLV_H
   int aclv_count;
-  struct acl aclv_entries[NACLVENTRIES];
+  struct acl aclv_entries[NACLVENTRIES + 1];
 # endif
   int did_chmod;
   int saved_errno;
   int ret;

-  for (;;)
-{
-  count = (source_desc != -1
-   ? fgetacl (source_desc, 0, NULL)
-   : getacl (src_name, 0, NULL));
-
-  if (count  0)
-{
-  if (errno == ENOSYS || errno == EOPNOTSUPP || errno == ENOTSUP)
-{
-  count = 0;
-  break;
-}
-  else
-return -2;
-}
+  count = (source_desc != -1
+   ? fgetacl (source_desc, NACLENTRIES + 1, entries)
+   : getacl (src_name, NACLENTRIES + 1, entries));

-  if (count == 0)
-break;
-
-  if (count  NACLENTRIES)
-/* If NACLENTRIES cannot be trusted, use dynamic memory allocation.  */
-abort ();
-
-  if ((source_desc !=

bug#10639: BUG REPORT coreutils-8.15 Solaris 10 64bit

2012-02-01 Thread Bruno Haible 
Paul Eggert wrote:
 Thanks, can you please try the attached patch?

I find it overkill to change the code for HP-UX and NonStop systems
when the report is about Solaris. Also I think the structure of the loop
is not the problem; it is the code's reaction to
  acl(x, ACE_GETACL, 4, 0x3432A16E0)Err#-1

Please, can you also try this patch?

2012-02-01  Bruno Haible  br...@clisp.org

* lib/file-has-acl.c (file_has_acl) [Solaris]: Treat a failing
acl()/facl() call for ACE_GETACL like a failing call for ACE_GETACLCNT.
* lib/set-mode-acl.c (qset_acl) [Solaris]: Likewise.
* lib/copy-acl.c (qcopy_acl)[Solaris]: Likewise.

--- lib/copy-acl.c.orig	Wed Feb  1 11:15:51 2012
+++ lib/copy-acl.c	Wed Feb  1 11:10:55 2012
@@ -235,10 +235,22 @@
   return -2;
 }
 
-  if ((source_desc != -1
-   ? facl (source_desc, ACE_GETACL, ace_count, ace_entries)
-   : acl (src_name, ACE_GETACL, ace_count, ace_entries))
-  == ace_count)
+  ret = (source_desc != -1
+ ? facl (source_desc, ACE_GETACL, ace_count, ace_entries)
+ : acl (src_name, ACE_GETACL, ace_count, ace_entries);
+  if (ret  0)
+{
+  free (ace_entries);
+  if (errno == ENOSYS || errno == EINVAL)
+{
+  ace_count = 0;
+  ace_entries = NULL;
+  break;
+}
+  else
+return -2;
+}
+  if (ret == ace_count)
 break;
   /* Huh? The number of ACL entries changed since the last call.
  Repeat.  */
--- lib/file-has-acl.c.orig	Wed Feb  1 11:15:51 2012
+++ lib/file-has-acl.c	Wed Feb  1 11:13:17 2012
@@ -588,6 +588,8 @@
 
 for (;;)
   {
+int ret;
+
 count = acl (name, ACE_GETACLCNT, 0, NULL);
 
 if (count  0)
@@ -618,7 +620,16 @@
 errno = ENOMEM;
 return -1;
   }
-if (acl (name, ACE_GETACL, count, entries) == count)
+ret = acl (name, ACE_GETACL, count, entries);
+if (ret  0)
+  {
+free (entries);
+if (errno == ENOSYS || errno == EINVAL)
+  break;
+else
+  return -1;
+  }
+if (ret == count)
   {
 if (acl_ace_nontrivial (count, entries))
   {
--- lib/set-mode-acl.c.orig	Wed Feb  1 11:15:51 2012
+++ lib/set-mode-acl.c	Wed Feb  1 11:15:26 2012
@@ -219,6 +219,8 @@
 
 for (;;)
   {
+int ret;
+
 if (desc != -1)
   count = facl (desc, ACE_GETACLCNT, 0, NULL);
 else
@@ -234,10 +236,16 @@
 errno = ENOMEM;
 return -1;
   }
-if ((desc != -1
- ? facl (desc, ACE_GETACL, count, entries)
- : acl (name, ACE_GETACL, count, entries))
-== count)
+ret = (desc != -1
+   ? facl (desc, ACE_GETACL, count, entries)
+   : acl (name, ACE_GETACL, count, entries));
+if (ret  0)
+  {
+free (entries);
+convention = -1;
+break;
+  }
+if (ret == count)
   {
 int i;

bug#10686: mv: moving hardlink of a softlink to the softlink does nothing

2012-02-01 Thread Bernhard Voelker 
On 02/01/2012 09:21 AM, Bernhard Voelker wrote:

   $ 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
 
 Trying to mv the hardlink over the symlink seems to succeed:
 
   $ ~/git/coreutils/src/mv s l
 
 ... but the name 's' was not unlinked:

...

 That behaviour didn't change at least since version 7.1
 (on openSuSE-11.3), but back in the earlier days in 5.93
 (SLES-10.3), 's' disappeared as expected:

Hmm, looks like commit
   f1d1ee91217d08527c6bf7682987339e38116268
introduced this in 2006.

Have a nice day,
Berny

bug#10686: mv: moving hardlink of a softlink to the softlink does nothing

2012-02-01 Thread Jim Meyering 
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.
For example, on FreeBSD 9.0, you cannot even create one:
(instead, it hard-links the referent of the symlink)

freebsd$ touch f  ln -s f l  ln l s  ls -ogi f l s
1587047 -rw-r--r-- 2 0 Feb  1 04:58 f
1587100 lrwxr-xr-x 1 1 Feb  1 04:58 l - f
1587047 -rw-r--r-- 2 0 Feb  1 04:58 s

 Trying to mv the hardlink over the symlink seems to succeed:

   $ ~/git/coreutils/src/mv s l

 ... but the name 's' was not unlinked:

That is definitely undesirable behavior.
For now, one possibility is to make mv reject
that corner case with this diagnostic:

mv: 's' and 'l' are the same file

Proposed patch below.

   $ 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

 Using the -v option looks also normal, but is a nop:

   $ ~/git/coreutils/src/mv -v s l
   ‘s’ - ‘l’
   $ 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

 The strace only shows the rename():

   stat(l, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
   lstat(s, {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
   lstat(l, {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
   rename(s, l)= 0


 I'd have expected either
 a) the name 's' to be unlinked, or
 b) an error message that something was wrong (well, whatever).
 I'd prefer a) of course.

So would I.

 That behaviour didn't change at least since version 7.1
 (on openSuSE-11.3), but back in the earlier days in 5.93
 (SLES-10.3), 's' disappeared as expected:

   stat(l, {st_mode=S_IFREG|0640, st_size=0, ...}) = 0
   lstat(s, {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
   lstat(l, {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
   unlink(l) = 0
   rename(s, l)= 0

 Does mv now work as specified? Is this a kernel or
 a coreutils (or a user) issue?

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.

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.

Here's a proposed patch.
This is such an improbable corner case that I think it's not worth trying
to fix any other way (either by unlinking the destination or by
replacing rename).

(of course, if I go with this, I'll insert a big comment justifying
this tiny change)

diff --git a/src/copy.c b/src/copy.c
index 4810de8..78cd8c8 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1229,7 +1229,7 @@ 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);
+return ! same_name (src_name, dst_name)  ! same_link;

   src_sb_link = src_sb;
   dst_sb_link = dst_sb;

bug#10686: mv: moving hardlink of a softlink to the softlink does nothing

2012-02-01 Thread Bernhard Voelker 
On 02/01/2012 01:47 PM, 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.
 For example, on FreeBSD 9.0, you cannot even create one:
 (instead, it hard-links the referent of the symlink)
 
 freebsd$ touch f  ln -s f l  ln l s  ls -ogi f l s
 1587047 -rw-r--r-- 2 0 Feb  1 04:58 f
 1587100 lrwxr-xr-x 1 1 Feb  1 04:58 l - f
 1587047 -rw-r--r-- 2 0 Feb  1 04:58 s
 
 Trying to mv the hardlink over the symlink seems to succeed:

   $ ~/git/coreutils/src/mv s l

 ... but the name 's' was not unlinked:
 
 That is definitely undesirable behavior.
 For now, one possibility is to make mv reject
 that corner case with this diagnostic:
 
 mv: 's' and 'l' are the same file
 
 Proposed patch below.
 
   $ 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

 Using the -v option looks also normal, but is a nop:

   $ ~/git/coreutils/src/mv -v s l
   ‘s’ - ‘l’
   $ 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

 The strace only shows the rename():

   stat(l, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
   lstat(s, {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
   lstat(l, {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
   rename(s, l)= 0


 I'd have expected either
 a) the name 's' to be unlinked, or
 b) an error message that something was wrong (well, whatever).
 I'd prefer a) of course.
 
 So would I.
 
 That behaviour didn't change at least since version 7.1
 (on openSuSE-11.3), but back in the earlier days in 5.93
 (SLES-10.3), 's' disappeared as expected:

   stat(l, {st_mode=S_IFREG|0640, st_size=0, ...}) = 0
   lstat(s, {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
   lstat(l, {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
   unlink(l) = 0
   rename(s, l)= 0

 Does mv now work as specified? Is this a kernel or
 a coreutils (or a user) issue?
 
 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.
 
 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.
 
 Here's a proposed patch.
 This is such an improbable corner case that I think it's not worth trying
 to fix any other way (either by unlinking the destination or by
 replacing rename).
 
 (of course, if I go with this, I'll insert a big comment justifying
 this tiny change)
 
 diff --git a/src/copy.c b/src/copy.c
 index 4810de8..78cd8c8 100644
 --- a/src/copy.c
 +++ b/src/copy.c
 @@ -1229,7 +1229,7 @@ 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);
 +return ! same_name (src_name, dst_name)  ! same_link;
 
src_sb_link = src_sb;
dst_sb_link = dst_sb;

Hi Jim,

well, if it's not standardized (yet), then I agree with you that we
should choose the simpler b) alternative.

Do you think it's work thinking about the -f case?

  $ ~/git/coreutils/src/mv -f s l
  /home/berny/git/coreutils/src/mv: ‘s’ and ‘l’ are the same file

The info pages are not quite clear in which cases mv tries to
overwrite the destination. Usually, I am tempted to think that
a --force will just do it - regardless of data loss.
But in this case, mv just does not and the only thing left
is to rm that hardlink.

That makes me think that the implementation of the a) alternative
just reduces to calling unlink() ... ;-)

Have a nice day,
Berny

bug#10686: mv: moving hardlink of a softlink to the softlink does nothing

2012-02-01 Thread Jim Meyering 
Bernhard Voelker wrote:
...
 well, if it's not standardized (yet), then I agree with you that we
 should choose the simpler b) alternative.

 Do you think it's work thinking about the -f case?

   $ ~/git/coreutils/src/mv -f s l
   /home/berny/git/coreutils/src/mv: ‘s’ and ‘l’ are the same file

mv's -f option is unrelated to this case.
It relates solely to controlling whether one is prompted
due to permissions or -i.

From 'info mv's description of the common case (no options):

   If a destination file exists but is normally unwritable, standard
input is a terminal, and the `-f' or `--force' option is not given,
`mv' prompts the user for whether to replace the file.  (You might own
the file, or have write permission on its directory.)  If the response
is not affirmative, the file is skipped.


Yes, it is unfortunate that the standards-imposed semantics
of -f is different from what we'd expect.

 The info pages are not quite clear in which cases mv tries to
 overwrite the destination. Usually, I am tempted to think that
 a --force will just do it - regardless of data loss.
 But in this case, mv just does not and the only thing left
 is to rm that hardlink.

If the above is still not clear, please suggest how to improve it.

 That makes me think that the implementation of the a) alternative
 just reduces to calling unlink() ... ;-)

bug#10686: mv: moving hardlink of a softlink to the softlink does nothing

2012-02-01 Thread Bernhard Voelker 


On 02/01/2012 03:02 PM, Jim Meyering wrote:
 Bernhard Voelker wrote:
 ...
 well, if it's not standardized (yet), then I agree with you that we
 should choose the simpler b) alternative.

 Do you think it's work thinking about the -f case?

   $ ~/git/coreutils/src/mv -f s l
   /home/berny/git/coreutils/src/mv: ‘s’ and ‘l’ are the same file
 
 mv's -f option is unrelated to this case.
 It relates solely to controlling whether one is prompted
 due to permissions or -i.
 
 From 'info mv's description of the common case (no options):
 
If a destination file exists but is normally unwritable, standard
 input is a terminal, and the `-f' or `--force' option is not given,
 `mv' prompts the user for whether to replace the file.  (You might own
 the file, or have write permission on its directory.)  If the response
 is not affirmative, the file is skipped.
 
 
 Yes, it is unfortunate that the standards-imposed semantics
 of -f is different from what we'd expect.
 

 If the above is still not clear, please suggest how to improve it.

Well, if permissions are the only case when mv would require
confirmation or -f, then it should be added to mv's description.

Additionally, the info page might also have note in which cases
mv will refuse to work, e.g. when ‘s’ and ‘l’ are the same file.

As I'm not sure (anymore) about both, I cannot propose a wording.


 That makes me think that the implementation of the a) alternative
 just reduces to calling unlink() ... ;-)

This hardlink-to-softlink case is IMHO identical to the normal
hardlinks case (in which mv just unlinks the source):

  $ touch f1  ln f1 f2  ls -ogi f1 f2
  6590 -rw-r--r-- 2 0 Feb  1 15:17 f1
  6590 -rw-r--r-- 2 0 Feb  1 15:17 f2
  $ ~/git/coreutils/src/mv f1 f2
  $ ls -ogi f1 f2
  ls: cannot access f1: No such file or directory
  6590 -rw-r--r-- 1 0 Feb  1 15:17 f2

Therefore, I'd tend to a) again. It doesn't matter if src is
a regular file or a symlink. What do you think?

Have a nice day,
Berny

bug#10686: mv: moving hardlink of a softlink to the softlink does nothing

2012-02-01 Thread Eric Blake 
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().

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

bug#10686: mv: moving hardlink of a softlink to the softlink does nothing

2012-02-01 Thread Jim Meyering 
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;
+}

   src_sb_link = src_sb;
   dst_sb_link = dst_sb;

bug#10686: mv: moving hardlink of a softlink to the softlink does nothing

2012-02-01 Thread Jim Meyering 
Bernhard Voelker wrote:

 On 02/01/2012 04:45 PM, Jim Meyering wrote:
 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;
 +}

src_sb_link = src_sb;
dst_sb_link = dst_sb;

 Thank you both.
 The patch works.

 I wonder if just removing the x-hard_link constraint at the beginning
 of same_file_ok() would alsoo do (although the comment sounds like this
 is no good idea):

 @@ -1213,11 +1213,11 @@ same_file_ok (char const *src_name, struct stat const 
 *src_sb,
/* FIXME: this should (at the very least) be moved into the following
   if-block.  More likely, it should be removed, because it inhibits
   making backups.  But removing it will result in a change in behavior
   that will probably have to be documented -- and tests will have to
   be updated.  */
 -  if (same  x-hard_link)
 +  if (same)

I doubt that would work (it doesn't set *unlink_src) -- though you're
welcome to try it.  Even if it did, I'd prefer not to put a change like
this in code that might be considered for removal.

*return_now = true;
return true;
  }

 Please don't think I just want to give you more work,
 but I think this deserves a new test, doesn't it?

Of course ;-)  That was not a complete patch by a long shot.
It didn't even have a commit log.  You obviously know by now that
any change like this absolutely requires an addition to the tests suite.
And since (thanks to Eric's clarification) it does officially count
as a bug fix, I'll write a NEWS entry, too.

Thanks again for spotting the problem.

bug#10639: BUG REPORT coreutils-8.15 Solaris 10 64bit

2012-02-01 Thread Paul Eggert 
On 02/01/2012 02:22 AM, Bruno Haible wrote:
 I find it overkill to change the code for HP-UX and NonStop systems
 when the report is about Solaris. Also I think the structure of the loop
 is not the problem; it is the code's reaction to
   acl(x, ACE_GETACL, 4, 0x3432A16E0)Err#-1

The patch you sent looks like it'll fix that particular bug, but while
looking into this I discovered so many bugs in this area, mostly
hard-to-test race conditions and overflows, that I thought it better
to rewrite the affected code than to try to fix each bug one at a time.
I didn't write this up very well, and my first cut at doing this could
stand some improvements of its own.  Here's a revised patch that tries
to do a better job at all this.  (If it's any consolation, this patch
makes the code simpler -- 186 lines shorter)


diff --git a/ChangeLog b/ChangeLog
index f80c7dd..ec16bef 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,89 @@
+2012-02-01  Paul Eggert  egg...@cs.ucla.edu
+
+   acl: fix several problems with ACLs
+   Problem with infinite loop on Solaris 10 + vxfs reported by Bill
+   Jones in http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10639.
+   Looking into the code, I saw several closely related issues:
+
+- There's a race condition in this kind of code:
+
+n = acl (f, GETACLCNT, 0, NULL);
+[ allocate an array A of size N ]
+if (acl (f, GETACL, n, a) == n)
+  return ok;
+
+  The ACL can grow in size between the first and second calls to
+  'acl', which means that the second 'acl' returns a truncated
+  ACL but thinks that it has the whole thing.  To avoid this, the
+  following pattern should be used instead:
+
+n = acl (f, GETACLCNT, 0, NULL);
+[ allocate an array A of size N + 1 ]
+n1 = acl (f, GETACL, n + 1, a);
+if (0  n1  n1 = n)
+  return ok;
+
+- There were several instances of this pattern:
+
+for (;;) {
+  n = acl (f, GETACLCNT, 0, NULL);
+  [ allocate an array A of size N ]
+  if (acl (f, GETACL, n, a) == n)
+break;
+}
+
+  This loop might never terminate if some other process is constantly
+  manipulating the file's ACL.  The loop should be rewritten to
+  terminate.
+
+- The acl (... GETACLNT ...) call is merely an optimization; its value
+  is merely a hint as to how big to make the array.  A better
+  optimization is to avoid the acl (... GETACLNT ...)  call entirely,
+  and just guess a reasonably-big size, growing the size and trying
+  again if it's not large enough.  This guarantees termination, and
+  saves a system call.
+
+- With this approach, for ports like HP-UX that have an upper bound
+  for the ACL length, there's no longer any need to loop reading the
+  ACL.  Just read it once and you're done reading it.
+
+- For ports like Solaris that do need to loop (because the ACL length
+  is not known a priori), it's faster to allocate a reasonably-sized
+  buffer on the stack and use that, and to allocate something on the
+  heap only if the ACL is unusually large.  This avoids malloc in the
+  usual case.
+
+- The code that calculated sizes of these arrays did not check for
+  overflow in size calculations; it should.
+
+- There are some memory leaks.  For example, in qcopy_acl, if acl
+  (src_name, GETACLCNT, 0, NULL)  0  errno == EPERM, then the
+  storage for ace_entries leaks.  Similarly, if acl (src_name,
+  ACE_GETACL, ace_count, ace_entries) returns -1, the storage for
+  ace_entries leaks.
+
+- In qset_acl, there's sometimes no need to read the entire ACL
+  to determine the convention; this can save system calls when
+  looking at very large ACLs.
+
+   Rather than fix these bugs one at a time I thought it more efficient
+   to rewrite the affected code, as follows:
+   * lib/acl-internal.h (GETACLCNT): Remove; no longer needed.
+   Include limits.h.
+   (MIN): New macro.
+   * lib/copy-acl.c (qcopy_acl): Don't bother to count ACL entries
+   before getting them.  Instead, just get them, and make sure that
+   the number of entries gotten is less than the number requested.
+   This avoids a race condition and omits a system call in the usual
+   case.  Before, it could have been the case that we asked for and
+   received N entries, but these were the first N of more than N
+   actual entries, if the ACL was modified while we were getting it.
+   With this change, there is no need to use GETACLCNT or similar ops.
+   Check for size-calculation overflow.
+   Avoid need for dynamic allocation if possible.
+   * lib/file-has-acl.c (file_has_acl):