From b1ab442e6db76de2a7315531f6f0ec59c5934368 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 4 Dec 2009 14:07:58 -0700 Subject: [PATCH] mgetgroups: reduce duplicate listings POSIX doesn't guarantee whether the effective gid is included in the list of supplementary groups returned by getgroups. On the other hand, some platforms include the effective gid twice in the list. Meanwhile, mgetgroups can independently add a duplicate. Rather than spend a full-blown O(n log n) cleanup, we just remove the most common forms of duplicate groups with an O(n) pass. * lib/mgetgroups.c (mgetgroups): Reduce duplicates from the resulting array. * tests/test-chown.h (test_chown): Simplify client. * tests/test-lchown.h (test_lchown): Likewise. Signed-off-by: Eric Blake --- ChangeLog | 8 ++++++++ lib/mgetgroups.c | 34 +++++++++++++++++++++++++++++++++- tests/test-chown.h | 27 +++++++++------------------ tests/test-lchown.h | 33 ++++++++++++--------------------- 4 files changed, 62 insertions(+), 40 deletions(-) diff --git a/ChangeLog b/ChangeLog index 977285c1fb..00ac8868c7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2009-12-07 Eric Blake + + mgetgroups: reduce duplicate listings + * lib/mgetgroups.c (mgetgroups): Reduce duplicates from the + resulting array. + * tests/test-chown.h (test_chown): Simplify client. + * tests/test-lchown.h (test_lchown): Likewise. + 2009-12-06 Bruno Haible * lib/cloexec.c (dup_cloexec): Fix handling of _gl_register_dup return diff --git a/lib/mgetgroups.c b/lib/mgetgroups.c index 7a61db41d9..5ca450f9da 100644 --- a/lib/mgetgroups.c +++ b/lib/mgetgroups.c @@ -53,7 +53,8 @@ realloc_groupbuf (gid_t *g, size_t num) NULL, store the supplementary groups of the current process, and GID should be -1 or the effective group ID (getegid). Upon failure, don't modify *GROUPS, set errno, and return -1. Otherwise, return - the number of groups. */ + the number of groups. The resulting list may contain duplicates, + but adjacent members will be distinct. */ int mgetgroups (char const *username, gid_t gid, gid_t **groups) @@ -157,6 +158,37 @@ mgetgroups (char const *username, gid_t gid, gid_t **groups) ng++; } *groups = g; + + /* Reduce the number of duplicates. On some systems, getgroups + returns the effective gid twice: once as the first element, and + once in its position within the supplementary groups. On other + systems, getgroups does not return the effective gid at all, + which is why we provide a GID argument. Meanwhile, the GID + argument, if provided, is typically any member of the + supplementary groups, and not necessarily the effective gid. So, + the most likely duplicates are the first element with an + arbitrary other element, or pair-wise duplication between the + first and second elements returned by getgroups. It is possible + that this O(n) pass will not remove all duplicates, but it is not + worth the effort to slow down to an O(n log n) algorithm that + sorts the array in place, nor the extra memory needed for + duplicate removal via an O(n) hash-table. Hence, this function + is only documented as guaranteeing no pair-wise duplicates, + rather than returning the minimal set. */ + { + gid_t first = *g; + gid_t *next; + gid_t *sentinel = g + ng; + + for (next = g + 1; next < sentinel; next++) + { + if (*next == first || *next == *g) + ng--; + else + *++g = *next; + } + } + return ng; } diff --git a/tests/test-chown.h b/tests/test-chown.h index 098b77d22b..c4e652af89 100644 --- a/tests/test-chown.h +++ b/tests/test-chown.h @@ -170,21 +170,12 @@ test_chown (int (*func) (char const *, uid_t, gid_t), bool print) changing group ownership of a file we own. If we belong to at least two groups, then verifying the correct change is simple. But if we belong to only one group, then we fall back on the - other observable effect of chown: the ctime must be updated. - Be careful of duplicates returned by getgroups. */ - gids_count = mgetgroups (NULL, -1, &gids); - if (2 <= gids_count && gids[0] == gids[1] && 2 < gids_count--) - gids[1] = gids[2]; - if (1 < gids_count || (gids_count == 1 && gids[0] != st1.st_gid)) + other observable effect of chown: the ctime must be updated. */ + gids_count = mgetgroups (NULL, st1.st_gid, &gids); + if (1 < gids_count) { - if (gids[0] == st1.st_gid) - { - ASSERT (1 < gids_count); - ASSERT (gids[0] != gids[1]); - gids[0] = gids[1]; - } - ASSERT (gids[0] != st1.st_gid); - ASSERT (gids[0] != (gid_t) -1); + ASSERT (gids[1] != st1.st_gid); + ASSERT (gids[1] != (gid_t) -1); ASSERT (lstat (BASE "dir/link", &st2) == 0); ASSERT (st1.st_uid == st2.st_uid); ASSERT (st1.st_gid == st2.st_gid); @@ -193,7 +184,7 @@ test_chown (int (*func) (char const *, uid_t, gid_t), bool print) ASSERT (st1.st_gid == st2.st_gid); errno = 0; - ASSERT (func (BASE "dir/link2/", -1, gids[0]) == -1); + ASSERT (func (BASE "dir/link2/", -1, gids[1]) == -1); ASSERT (errno == ENOTDIR); ASSERT (stat (BASE "dir/file", &st2) == 0); ASSERT (st1.st_uid == st2.st_uid); @@ -205,10 +196,10 @@ test_chown (int (*func) (char const *, uid_t, gid_t), bool print) ASSERT (st1.st_uid == st2.st_uid); ASSERT (st1.st_gid == st2.st_gid); - ASSERT (func (BASE "dir/link2", -1, gids[0]) == 0); + ASSERT (func (BASE "dir/link2", -1, gids[1]) == 0); ASSERT (stat (BASE "dir/file", &st2) == 0); ASSERT (st1.st_uid == st2.st_uid); - ASSERT (gids[0] == st2.st_gid); + ASSERT (gids[1] == st2.st_gid); ASSERT (lstat (BASE "dir/link", &st2) == 0); ASSERT (st1.st_uid == st2.st_uid); ASSERT (st1.st_gid == st2.st_gid); @@ -238,7 +229,7 @@ test_chown (int (*func) (char const *, uid_t, gid_t), bool print) ASSERT (l2.st_ctime == st2.st_ctime); ASSERT (get_stat_ctime_ns (&l2) == get_stat_ctime_ns (&st2)); - ASSERT (func (BASE "dir/link2", -1, gids[0]) == 0); + ASSERT (func (BASE "dir/link2", -1, st1.st_gid) == 0); ASSERT (stat (BASE "dir/file", &st2) == 0); ASSERT (st1.st_ctime < st2.st_ctime || (st1.st_ctime == st2.st_ctime diff --git a/tests/test-lchown.h b/tests/test-lchown.h index 43690ac4c6..93a657cf5d 100644 --- a/tests/test-lchown.h +++ b/tests/test-lchown.h @@ -189,21 +189,12 @@ test_lchown (int (*func) (char const *, uid_t, gid_t), bool print) changing group ownership of a file we own. If we belong to at least two groups, then verifying the correct change is simple. But if we belong to only one group, then we fall back on the - other observable effect of lchown: the ctime must be updated. - Be careful of duplicates returned by getgroups. */ - gids_count = mgetgroups (NULL, -1, &gids); - if (2 <= gids_count && gids[0] == gids[1] && 2 < gids_count--) - gids[1] = gids[2]; - if (1 < gids_count || (gids_count == 1 && gids[0] != st1.st_gid)) + other observable effect of lchown: the ctime must be updated. */ + gids_count = mgetgroups (NULL, st1.st_gid, &gids); + if (1 < gids_count) { - if (gids[0] == st1.st_gid) - { - ASSERT (1 < gids_count); - ASSERT (gids[0] != gids[1]); - gids[0] = gids[1]; - } - ASSERT (gids[0] != st1.st_gid); - ASSERT (gids[0] != (gid_t) -1); + ASSERT (gids[1] != st1.st_gid); + ASSERT (gids[1] != (gid_t) -1); ASSERT (lstat (BASE "dir/link", &st2) == 0); ASSERT (st1.st_uid == st2.st_uid); ASSERT (st1.st_gid == st2.st_gid); @@ -212,7 +203,7 @@ test_lchown (int (*func) (char const *, uid_t, gid_t), bool print) ASSERT (st1.st_gid == st2.st_gid); errno = 0; - ASSERT (func (BASE "dir/link2/", -1, gids[0]) == -1); + ASSERT (func (BASE "dir/link2/", -1, gids[1]) == -1); ASSERT (errno == ENOTDIR); ASSERT (stat (BASE "dir/file", &st2) == 0); ASSERT (st1.st_uid == st2.st_uid); @@ -224,7 +215,7 @@ test_lchown (int (*func) (char const *, uid_t, gid_t), bool print) ASSERT (st1.st_uid == st2.st_uid); ASSERT (st1.st_gid == st2.st_gid); - ASSERT (func (BASE "dir/link2", -1, gids[0]) == 0); + ASSERT (func (BASE "dir/link2", -1, gids[1]) == 0); ASSERT (stat (BASE "dir/file", &st2) == 0); ASSERT (st1.st_uid == st2.st_uid); ASSERT (st1.st_gid == st2.st_gid); @@ -233,7 +224,7 @@ test_lchown (int (*func) (char const *, uid_t, gid_t), bool print) ASSERT (st1.st_gid == st2.st_gid); ASSERT (lstat (BASE "dir/link2", &st2) == 0); ASSERT (st1.st_uid == st2.st_uid); - ASSERT (gids[0] == st2.st_gid); + ASSERT (gids[1] == st2.st_gid); /* Trailing slash follows through to directory. */ ASSERT (lstat (BASE "dir/link3", &st2) == 0); @@ -243,13 +234,13 @@ test_lchown (int (*func) (char const *, uid_t, gid_t), bool print) ASSERT (st1.st_uid == st2.st_uid); ASSERT (st1.st_gid == st2.st_gid); - ASSERT (func (BASE "dir/link3/", -1, gids[0]) == 0); + ASSERT (func (BASE "dir/link3/", -1, gids[1]) == 0); ASSERT (lstat (BASE "dir/link3", &st2) == 0); ASSERT (st1.st_uid == st2.st_uid); ASSERT (st1.st_gid == st2.st_gid); ASSERT (lstat (BASE "dir/sub", &st2) == 0); ASSERT (st1.st_uid == st2.st_uid); - ASSERT (gids[0] == st2.st_gid); + ASSERT (gids[1] == st2.st_gid); } else if (!CHOWN_CHANGE_TIME_BUG || HAVE_LCHMOD) { @@ -275,7 +266,7 @@ test_lchown (int (*func) (char const *, uid_t, gid_t), bool print) ASSERT (l2.st_ctime == st2.st_ctime); ASSERT (get_stat_ctime_ns (&l2) == get_stat_ctime_ns (&st2)); - ASSERT (func (BASE "dir/link2", -1, gids[0]) == 0); + ASSERT (func (BASE "dir/link2", -1, st1.st_gid) == 0); ASSERT (stat (BASE "dir/file", &st2) == 0); ASSERT (st1.st_ctime == st2.st_ctime); ASSERT (get_stat_ctime_ns (&st1) == get_stat_ctime_ns (&st2)); @@ -291,7 +282,7 @@ test_lchown (int (*func) (char const *, uid_t, gid_t), bool print) ASSERT (lstat (BASE "dir/sub", &st1) == 0); ASSERT (lstat (BASE "dir/link3", &l1) == 0); nap (); - ASSERT (func (BASE "dir/link3/", -1, gids[0]) == 0); + ASSERT (func (BASE "dir/link3/", -1, st1.st_gid) == 0); ASSERT (lstat (BASE "dir/link3", &st2) == 0); ASSERT (l1.st_ctime == st2.st_ctime); ASSERT (get_stat_ctime_ns (&l1) == get_stat_ctime_ns (&st2)); -- 2.30.2