changeset 16382:cc05dad27529

acl: Don't use GETACLCNT and similar ops, since they are unreliable. - 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. * lib/acl-internal.h: Include <limits.h>. (MIN, SIZE_MAX): New macros. * lib/file-has-acl.c (file_has_acl) [Solaris]: Read the entries into a stack-allocated buffer, and use malloc if it does not fit. Don't use GETACLCNT. * lib/set-mode-acl.c (qset_acl) [Solaris]: Likewise.
author Paul Eggert <eggert@cs.ucla.edu>
date Mon, 20 Feb 2012 01:12:06 +0100
parents fc5c37ccbece
children 531aa00a1e80
files ChangeLog lib/acl-internal.h lib/file-has-acl.c lib/set-mode-acl.c
diffstat 4 files changed, 200 insertions(+), 115 deletions(-) [+]
line wrap: on
line diff
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,35 @@
+2012-02-19  Paul Eggert  <eggert@cs.ucla.edu>
+	    Bruno Haible  <bruno@clisp.org>
+
+	acl: Don't use GETACLCNT and similar ops, since they are unreliable.
+
+	 - 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.
+
+	* lib/acl-internal.h: Include <limits.h>.
+	(MIN, SIZE_MAX): New macros.
+	* lib/file-has-acl.c (file_has_acl) [Solaris]: Read the entries into
+	a stack-allocated buffer, and use malloc if it does not fit. Don't
+	use GETACLCNT.
+	* lib/set-mode-acl.c (qset_acl) [Solaris]: Likewise.
+
 2012-02-19  Bruno Haible  <bruno@clisp.org>
 
 	acl: Fix endless loop on Solaris with vxfs.
--- a/lib/acl-internal.h
+++ b/lib/acl-internal.h
@@ -55,6 +55,15 @@
 # define ENOTSUP (-1)
 #endif
 
+#include <limits.h>
+#ifndef MIN
+# define MIN(a,b) ((a) < (b) ? (a) : (b))
+#endif
+
+#ifndef SIZE_MAX
+# define SIZE_MAX ((size_t) -1)
+#endif
+
 #ifndef HAVE_FCHMOD
 # define HAVE_FCHMOD false
 # define fchmod(fd, mode) (-1)
--- a/lib/file-has-acl.c
+++ b/lib/file-has-acl.c
@@ -556,7 +556,7 @@
         return ACL_NOT_WELL_SUPPORTED (errno) ? 0 : -1;
       return ret;
 
-# elif HAVE_FACL && defined GETACLCNT /* Solaris, Cygwin, not HP-UX */
+# elif HAVE_FACL && defined GETACL /* Solaris, Cygwin, not HP-UX */
 
 #  if defined ACL_NO_TRIVIAL
 
@@ -570,77 +570,135 @@
       /* Solaris 2.5 through Solaris 10, Cygwin, and contemporaneous versions
          of Unixware.  The acl() call returns the access and default ACL both
          at once.  */
-      int count;
       {
-        aclent_t *entries;
+        /* Initially, try to read the entries into a stack-allocated buffer.
+           Use malloc if it does not fit.  */
+        enum
+          {
+            alloc_init = 4000 / sizeof (aclent_t), /* >= 3 */
+            alloc_max = MIN (INT_MAX, SIZE_MAX / sizeof (aclent_t))
+          };
+        aclent_t buf[alloc_init];
+        size_t alloc = alloc_init;
+        aclent_t *entries = buf;
+        aclent_t *malloced = NULL;
+        int count;
 
         for (;;)
           {
-            count = acl (name, GETACLCNT, 0, NULL);
-
-            if (count < 0)
+            count = acl (name, GETACL, alloc, entries);
+            if (count < 0 && errno == ENOSPC)
               {
-                if (errno == ENOSYS || errno == ENOTSUP)
-                  break;
-                else
-                  return -1;
+                /* Increase the size of the buffer.  */
+                free (malloced);
+                if (alloc > alloc_max / 2)
+                  {
+                    errno = ENOMEM;
+                    return -1;
+                  }
+                alloc = 2 * alloc; /* <= alloc_max */
+                entries = malloced =
+                  (aclent_t *) malloc (alloc * sizeof (aclent_t));
+                if (entries == NULL)
+                  {
+                    errno = ENOMEM;
+                    return -1;
+                  }
+                continue;
               }
-
-            if (count == 0)
-              break;
-
+            break;
+          }
+        if (count < 0)
+          {
+            if (errno == ENOSYS || errno == ENOTSUP)
+              ;
+            else
+              {
+                int saved_errno = errno;
+                free (malloced);
+                errno = saved_errno;
+                return -1;
+              }
+          }
+        else if (count == 0)
+          ;
+        else
+          {
             /* Don't use MIN_ACL_ENTRIES:  It's set to 4 on Cygwin, but Cygwin
                returns only 3 entries for files with no ACL.  But this is safe:
                If there are more than 4 entries, there cannot be only the
                "user::", "group::", "other:", and "mask:" entries.  */
             if (count > 4)
-              return 1;
-
-            entries = (aclent_t *) malloc (count * sizeof (aclent_t));
-            if (entries == NULL)
-              {
-                errno = ENOMEM;
-                return -1;
-              }
-            if (acl (name, GETACL, count, entries) == count)
               {
-                if (acl_nontrivial (count, entries))
-                  {
-                    free (entries);
-                    return 1;
-                  }
-                free (entries);
-                break;
+                free (malloced);
+                return 1;
               }
-            /* Huh? The number of ACL entries changed since the last call.
-               Repeat.  */
-            free (entries);
+
+            if (acl_nontrivial (count, entries))
+              {
+                free (malloced);
+                return 1;
+              }
           }
+        free (malloced);
       }
 
 #   ifdef ACE_GETACL
       /* Solaris also has a different variant of ACLs, used in ZFS and NFSv4
          file systems (whereas the other ones are used in UFS file systems).  */
       {
-        ace_t *entries;
+        /* Initially, try to read the entries into a stack-allocated buffer.
+           Use malloc if it does not fit.  */
+        enum
+          {
+            alloc_init = 4000 / sizeof (ace_t), /* >= 3 */
+            alloc_max = MIN (INT_MAX, SIZE_MAX / sizeof (ace_t))
+          };
+        ace_t buf[alloc_init];
+        size_t alloc = alloc_init;
+        ace_t *entries = buf;
+        ace_t *malloced = NULL;
+        int count;
 
         for (;;)
           {
-            int ret;
-
-            count = acl (name, ACE_GETACLCNT, 0, NULL);
-
-            if (count < 0)
+            count = acl (name, ACE_GETACL, alloc, entries);
+            if (count < 0 && errno == ENOSPC)
               {
-                if (errno == ENOSYS || errno == EINVAL)
-                  break;
-                else
-                  return -1;
+                /* Increase the size of the buffer.  */
+                free (malloced);
+                if (alloc > alloc_max / 2)
+                  {
+                    errno = ENOMEM;
+                    return -1;
+                  }
+                alloc = 2 * alloc; /* <= alloc_max */
+                entries = malloced = (ace_t *) malloc (alloc * sizeof (ace_t));
+                if (entries == NULL)
+                  {
+                    errno = ENOMEM;
+                    return -1;
+                  }
+                continue;
               }
-
-            if (count == 0)
-              break;
-
+            break;
+          }
+        if (count < 0)
+          {
+            if (errno == ENOSYS || errno == EINVAL)
+              ;
+            else
+              {
+                int saved_errno = errno;
+                free (malloced);
+                errno = saved_errno;
+                return -1;
+              }
+          }
+        else if (count == 0)
+          ;
+        else
+          {
             /* In the old (original Solaris 10) convention:
                If there are more than 3 entries, there cannot be only the
                ACE_OWNER, ACE_GROUP, ACE_OTHER entries.
@@ -650,37 +708,18 @@
                NEW_ACE_ACCESS_ALLOWED_ACE_TYPE and once with
                NEW_ACE_ACCESS_DENIED_ACE_TYPE.  */
             if (count > 6)
-              return 1;
-
-            entries = (ace_t *) malloc (count * sizeof (ace_t));
-            if (entries == NULL)
-              {
-                errno = ENOMEM;
-                return -1;
-              }
-            ret = acl (name, ACE_GETACL, count, entries);
-            if (ret < 0)
               {
-                free (entries);
-                if (errno == ENOSYS || errno == EINVAL)
-                  break;
-                else
-                  return -1;
+                free (malloced);
+                return 1;
               }
-            if (ret == count)
+
+            if (acl_ace_nontrivial (count, entries))
               {
-                if (acl_ace_nontrivial (count, entries))
-                  {
-                    free (entries);
-                    return 1;
-                  }
-                free (entries);
-                break;
+                free (malloced);
+                return 1;
               }
-            /* Huh? The number of ACL entries changed since the last call.
-               Repeat.  */
-            free (entries);
           }
+        free (malloced);
       }
 #   endif
 
--- a/lib/set-mode-acl.c
+++ b/lib/set-mode-acl.c
@@ -197,7 +197,7 @@
   return chmod_or_fchmod (name, desc, mode);
 #  endif
 
-# elif HAVE_FACL && defined GETACLCNT /* Solaris, Cygwin, not HP-UX */
+# elif HAVE_FACL && defined GETACL /* Solaris, Cygwin, not HP-UX */
 
   int done_setacl = 0;
 
@@ -214,55 +214,60 @@
   int convention;
 
   {
+    /* Initially, try to read the entries into a stack-allocated buffer.
+       Use malloc if it does not fit.  */
+    enum
+      {
+        alloc_init = 4000 / sizeof (ace_t), /* >= 3 */
+        alloc_max = MIN (INT_MAX, SIZE_MAX / sizeof (ace_t))
+      };
+    ace_t buf[alloc_init];
+    size_t alloc = alloc_init;
+    ace_t *entries = buf;
+    ace_t *malloced = NULL;
     int count;
-    ace_t *entries;
 
     for (;;)
       {
-        int ret;
-
-        if (desc != -1)
-          count = facl (desc, ACE_GETACLCNT, 0, NULL);
-        else
-          count = acl (name, ACE_GETACLCNT, 0, NULL);
-        if (count <= 0)
-          {
-            convention = -1;
-            break;
-          }
-        entries = (ace_t *) malloc (count * sizeof (ace_t));
-        if (entries == NULL)
-          {
-            errno = ENOMEM;
-            return -1;
-          }
-        ret = (desc != -1
-               ? facl (desc, ACE_GETACL, count, entries)
-               : acl (name, ACE_GETACL, count, entries));
-        if (ret < 0)
+        count = (desc != -1
+                 ? facl (desc, ACE_GETACL, alloc, entries)
+                 : acl (name, ACE_GETACL, alloc, entries));
+        if (count < 0 && errno == ENOSPC)
           {
-            free (entries);
-            convention = -1;
-            break;
+            /* Increase the size of the buffer.  */
+            free (malloced);
+            if (alloc > alloc_max / 2)
+              {
+                errno = ENOMEM;
+                return -1;
+              }
+            alloc = 2 * alloc; /* <= alloc_max */
+            entries = malloced = (ace_t *) malloc (alloc * sizeof (ace_t));
+            if (entries == NULL)
+              {
+                errno = ENOMEM;
+                return -1;
+              }
+            continue;
           }
-        if (ret == count)
-          {
-            int i;
+        break;
+      }
 
-            convention = 0;
-            for (i = 0; i < count; i++)
-              if (entries[i].a_flags & (OLD_ACE_OWNER | OLD_ACE_GROUP | OLD_ACE_OTHER))
-                {
-                  convention = 1;
-                  break;
-                }
-            free (entries);
-            break;
-          }
-        /* Huh? The number of ACL entries changed since the last call.
-           Repeat.  */
-        free (entries);
+    if (count <= 0)
+      convention = -1;
+    else
+      {
+        int i;
+
+        convention = 0;
+        for (i = 0; i < count; i++)
+          if (entries[i].a_flags & (OLD_ACE_OWNER | OLD_ACE_GROUP | OLD_ACE_OTHER))
+            {
+              convention = 1;
+              break;
+            }
       }
+    free (malloced);
   }
 
   if (convention >= 0)