From c78091bebe80acf4b14d460999b2b2a9cea15b28 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 20 Feb 2012 01:12:06 +0100 Subject: [PATCH] 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 . (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. --- ChangeLog | 32 ++++++++++ lib/acl-internal.h | 9 +++ lib/file-has-acl.c | 181 ++++++++++++++++++++++++++++++++--------------------- lib/set-mode-acl.c | 89 +++++++++++++------------- 4 files changed, 198 insertions(+), 113 deletions(-) diff --git a/ChangeLog b/ChangeLog index 851f4fc68..0af5d80c3 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,35 @@ +2012-02-19 Paul Eggert + Bruno Haible + + 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 . + (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 acl: Fix endless loop on Solaris with vxfs. diff --git a/lib/acl-internal.h b/lib/acl-internal.h index 88e5e4598..8c4121932 100644 --- a/lib/acl-internal.h +++ b/lib/acl-internal.h @@ -55,6 +55,15 @@ extern int aclsort (int, int, struct acl *); # define ENOTSUP (-1) #endif +#include +#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) diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c index 77822f040..5d3e9e48c 100644 --- a/lib/file-has-acl.c +++ b/lib/file-has-acl.c @@ -556,7 +556,7 @@ file_has_acl (char const *name, struct stat const *sb) 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 @@ file_has_acl (char const *name, struct stat const *sb) /* 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; + free (malloced); + return 1; } - if (acl (name, GETACL, count, entries) == count) + + if (acl_nontrivial (count, entries)) { - 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); } + 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 @@ file_has_acl (char const *name, struct stat const *sb) 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 diff --git a/lib/set-mode-acl.c b/lib/set-mode-acl.c index 7fb0890ee..25a1eeee2 100644 --- a/lib/set-mode-acl.c +++ b/lib/set-mode-acl.c @@ -197,7 +197,7 @@ qset_acl (char const *name, int desc, mode_t mode) 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 @@ qset_acl (char const *name, int desc, mode_t mode) 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) + count = (desc != -1 + ? facl (desc, ACE_GETACL, alloc, entries) + : acl (name, ACE_GETACL, alloc, entries)); + if (count < 0 && errno == ENOSPC) { - convention = -1; - break; - } - entries = (ace_t *) malloc (count * sizeof (ace_t)); - if (entries == NULL) - { - errno = ENOMEM; - 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; } - 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; + 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) -- 2.11.0