On 02/19/2012 07:18 PM, Bruno Haible wrote:
> You would think that commit is fine, right? But it introduces a test failure
> on Solaris 10 over NFS:
>
> files tmpfile0 and tmpfile2: different ACE-ACL entry #0: different access
> masks 1400207 and 5400607
> FAIL: test-copy-acl-2.sh
>
> What's happening, is that the test-copy-acl copies wrong ACL entries.
Thanks for looking into this.
I am not observing this problem on my Solaris 10 + NFS host.
$ uname -a
SunOS kiwi.cs.ucla.edu 5.10 Generic_141444-09 sun4u sparc SUNW,Sun-Fire-280R
Solaris
$ mount | grep eggert
/home/eggert on elephant:/export/elephant/f/eggert
remote/read/write/setuid/devices/xattr/dev=55522d5 on Wed Feb 22 10:24:33 2012
For file-has-acl.c I'd like it if the code didn't have the arbitrary limit
around half the maximum allocated size, and the code could be simplified
and regularized a bit so that the two Solaris cases are as similar as
possible, so I propose the attached patch. The same idea can be applied to
copy-acl.c but of course it's more important to fix the bug.
(Maybe applying the idea will fix the bug? ...)
>From b9b3ebce606b745dd52564dd0534859365eb8534 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Wed, 22 Feb 2012 11:08:08 -0800
Subject: [PATCH] acl: unlimit and simplify file_has_acl
* lib/file-has-acl.c (file_has_acl): Try all the way up to the
maximum possible allocation rather than giving up half way.
This should have little or no practical effect, but it's nicer
to have the code be clear about the limits. Also, simplify
the Solaris, Cygwin, general case code a bit.
---
ChangeLog | 9 +++
lib/file-has-acl.c | 177 ++++++++++++++++++++--------------------------------
2 files changed, 76 insertions(+), 110 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 444422e..dcfce89 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2012-02-22 Paul Eggert <egg...@cs.ucla.edu>
+
+ acl: unlimit and simplify file_has_acl
+ * lib/file-has-acl.c (file_has_acl): Try all the way up to the
+ maximum possible allocation rather than giving up half way.
+ This should have little or no practical effect, but it's nicer
+ to have the code be clear about the limits. Also, simplify
+ the Solaris, Cygwin, general case code a bit.
+
2012-02-20 Paul Eggert <egg...@cs.ucla.edu>
regex: fix typo in definition of MIN
diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
index 6b17678..4a23790 100644
--- a/lib/file-has-acl.c
+++ b/lib/file-has-acl.c
@@ -573,74 +573,52 @@ file_has_acl (char const *name, struct stat const *sb)
{
/* Initially, try to read the entries into a stack-allocated buffer.
Use malloc if it does not fit. */
+ aclent_t *entries;
enum
{
- alloc_init = 4000 / sizeof (aclent_t), /* >= 3 */
- alloc_max = MIN (INT_MAX, SIZE_MAX / sizeof (aclent_t))
+ alloc_init = 4000 / sizeof *entries, /* >= 3 */
+ alloc_max = MIN (INT_MAX, SIZE_MAX / sizeof *entries)
};
aclent_t buf[alloc_init];
size_t alloc = alloc_init;
- aclent_t *entries = buf;
- aclent_t *malloced = NULL;
- int count;
+ int ret, count;
+ entries = buf;
- for (;;)
+ while ((count = acl (name, GETACL, alloc, entries)) < 0
+ && errno == ENOSPC)
{
- count = acl (name, GETACL, alloc, entries);
- if (count < 0 && errno == ENOSPC)
+ /* Increase the size of the buffer. */
+ if (entries != buf)
+ free (entries);
+ if (alloc == alloc_max)
+ entries = NULL;
+ else
{
- /* 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;
+ alloc = alloc <= alloc_max / 2 ? 2 * alloc : alloc_max;
+ entries = malloc (alloc * sizeof *entries);
}
- break;
- }
- if (count < 0)
- {
- if (errno == ENOSYS || errno == ENOTSUP)
- ;
- else
+ if (! entries)
{
- int saved_errno = errno;
- free (malloced);
- errno = saved_errno;
+ errno = ENOMEM;
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)
- {
- free (malloced);
- return 1;
- }
- if (acl_nontrivial (count, entries))
- {
- free (malloced);
- return 1;
- }
+ /* 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. */
+ ret = (count < 0
+ ? (errno == ENOSYS || errno == ENOTSUP ? 0 : -1)
+ : 4 < count || acl_nontrivial (count, entries));
+ if (entries != buf)
+ {
+ int saved_errno = errno;
+ free (entries);
+ errno = saved_errno;
}
- free (malloced);
+ if (ret)
+ return ret;
}
# ifdef ACE_GETACL
@@ -649,77 +627,56 @@ file_has_acl (char const *name, struct stat const *sb)
{
/* Initially, try to read the entries into a stack-allocated buffer.
Use malloc if it does not fit. */
+ ace_t *entries;
enum
{
- alloc_init = 4000 / sizeof (ace_t), /* >= 3 */
- alloc_max = MIN (INT_MAX, SIZE_MAX / sizeof (ace_t))
+ alloc_init = 4000 / sizeof *entries, /* >= 3 */
+ alloc_max = MIN (INT_MAX, SIZE_MAX / sizeof *entries)
};
ace_t buf[alloc_init];
size_t alloc = alloc_init;
- ace_t *entries = buf;
- ace_t *malloced = NULL;
- int count;
+ int ret, count;
+ entries = buf;
- for (;;)
+ while ((count = acl (name, ACE_GETACL, alloc, entries)) < 0
+ && errno == ENOSPC)
{
- count = acl (name, ACE_GETACL, alloc, entries);
- if (count < 0 && errno == ENOSPC)
+ /* Increase the size of the buffer. */
+ if (entries != buf)
+ free (entries);
+ if (alloc == alloc_max)
+ entries = NULL;
+ else
{
- /* 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;
+ alloc = alloc <= alloc_max / 2 ? 2 * alloc : alloc_max;
+ entries = malloc (alloc * sizeof *entries);
}
- break;
- }
- if (count < 0)
- {
- if (errno == ENOSYS || errno == EINVAL)
- ;
- else
+ if (! entries)
{
- int saved_errno = errno;
- free (malloced);
- errno = saved_errno;
+ errno = ENOMEM;
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.
- In the newer Solaris 10 and Solaris 11 convention:
- If there are more than 6 entries, there cannot be only the
- ACE_OWNER, ACE_GROUP, ACE_EVERYONE entries, each once with
- NEW_ACE_ACCESS_ALLOWED_ACE_TYPE and once with
- NEW_ACE_ACCESS_DENIED_ACE_TYPE. */
- if (count > 6)
- {
- free (malloced);
- return 1;
- }
- if (acl_ace_nontrivial (count, entries))
- {
- free (malloced);
- return 1;
- }
+ /* 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.
+ In the newer Solaris 10 and Solaris 11 convention:
+ If there are more than 6 entries, there cannot be only the
+ ACE_OWNER, ACE_GROUP, ACE_EVERYONE entries, each once with
+ NEW_ACE_ACCESS_ALLOWED_ACE_TYPE and once with
+ NEW_ACE_ACCESS_DENIED_ACE_TYPE. */
+ ret = (count < 0
+ ? (errno == ENOSYS || errno == EINVAL ? 0 : -1)
+ : 6 < count || acl_ace_nontrivial (count, entries));
+ if (entries != buf)
+ {
+ int saved_errno = errno;
+ free (entries);
+ errno = saved_errno;
}
- free (malloced);
+ if (ret)
+ return ret;
}
# endif
@@ -833,7 +790,7 @@ file_has_acl (char const *name, struct stat const *sb)
}
return -1;
}
- aclsize = 2 * aclsize;
+ aclsize = aclsize <= SIZE_MAX / 2 ? 2 * aclsize : SIZE_MAX;
if (acl != aclbuf)
free (acl);
acl = malloc (aclsize);
--
1.7.6.5
