On Saturday 27 September 2008 17:06, Tito wrote:
> On Saturday 27 September 2008 16:45:47 you wrote:
> > On Saturday 27 September 2008 16:00, Tito wrote:
> > > Hi Denys,
> > > maybe this patches slipped through as my cc to you get
> > > bounced by a spam filter that blacklisted my ip?
> >
> > I received them. sorry about this delay.
> >
> > > # [PATCH 1/3] add bb_getgrouplist_malloc to libbb Tito
> > > http://www.busybox.net/lists/busybox/2008-September/033078.html
> > > # [PATCH 2/3] port id to bb_getgrouplist_malloc + fixes Tito
> > > http://www.busybox.net/lists/busybox/2008-September/033079.html
> >
> > Let's look at these two patches together.
> >
> > Here:
> >
> > if (username) {
> > -#if HAVE_getgrouplist
> > - int m;
> > -#endif
> > p = getpwnam(username);
> > /* xuname2uid is needed because it exits on failure */
> > uid = xuname2uid(username);
> > gid = p->pw_gid; /* in this case PRINT_REAL is the same */
> > -
> > -#if HAVE_getgrouplist
> > - n = 16;
> > - groups = NULL;
> > - do {
> > - m = n;
> > - groups = xrealloc(groups, sizeof(groups[0]) * m);
> > - getgrouplist(username, gid, groups, &n); /* GNUism?
> > */
> > - } while (n > m);
> > -#endif
> > - } else {
> > -#if HAVE_getgrouplist
> > - n = getgroups(0, NULL);
> > - groups = xmalloc(sizeof(groups[0]) * n);
> > - getgroups(n, groups);
> > -#endif
> > }
> > + group_list = bb_getgrouplist_malloc(uid, gid, NULL);
> >
> > you replace getgrouplist() call with bb_getgrouplist_malloc().
> > bb_getgrouplist_malloc() iterates through the whole set of group records:
> >
> > + while((grp = getgrent())) {
> > ...
> > + while (*(grp->gr_mem)) {
> > ...
> > + }
> > + }
> >
> > This may be very expensive. I know for the fact than in big organizations
> > some groups are HUGE - I saw 500kbytes large "guests" group.
> > (nscd died horribly trying to digest such a large group.)
> >
> > In these cases, user db is not in /etc/XXX files. It is in LDAP etc.
> >
> > getgrouplist() may have a more clever way of retrieving this data -
> > instead of pulling megabytes from user datrabase by iterating through
> > all groups it may just directly query uer db "give me group list
> > of this user". Likely to be less than kilobyte of data.
> >
> > There is no way around it. If you want to make sure you have at least
> > a fleeting chance of not performing horribly, you must use libc interfaces
> > fro retrieving group lists, of which there is two known to me:
> > initgroups (standard, but useless in many ceses) and getgrouplist (GNUism).
> > Then, *if* your libc is well-written, it will hopefully do it efficiently.
>
> Hi,
> getgrent in fact is used only if getgrouplist is not available as suggested
> by Bernard,
> see #ifdef HAVE_getgrouplist
I missed that.
> > Secondly, make bloatcheck says:
> >
> > function old new delta
> > bb_getgrouplist_malloc - 192 +192
> > print_single - 106 +106
> > print_group_list - 94 +94
> > decode_format_string 824 839 +15
> > ...(deleted gcc-induced random jitter +/-9 bytes)...
> > printf_full 44 - -44
> > id_main 539 331 -208
> > ------------------------------------------------------------------------------
> > (add/remove: 3/1 grow/shrink: 4/4 up/down: 418/-259) Total: 159
> > bytes
> >
> > This does not seem to be a progress.
> I suppose +159 is with all 3 patches applied?
This was with only two first patches. I retested it on glibc, where
getgrouplist() is available:
function old new delta
bb_getgrouplist_malloc - 194 +194
print_single - 106 +106
print_group_list - 94 +94
bbunpack 386 394 +8
parse_command 1443 1440 -3
.rodata 125866 125852 -14
dpkg_main 2853 2836 -17
printf_full 44 - -44
id_main 541 331 -210
------------------------------------------------------------------------------
(add/remove: 3/1 grow/shrink: 1/4 up/down: 402/-288) Total: 114 bytes
> In this case bloat check is not so important as we are adding a function
> to libbb that could be reused in other applets and the new function
> has more features as the previous code: built in egid handling.
It's difficult to see how much of the growth is because of fixes
and how much because of bb_getgrouplist_malloc. Can you
submit them separately, so that it is possible
to measure them separately?
Meanwhile, I propose adding getgrouplist() to uclibc. See attached.
--
vda
diff -d -urpN -U5 uClibc.0/include/grp.h uClibc.1/include/grp.h
--- uClibc.0/include/grp.h 2008-09-27 16:49:39.000000000 +0200
+++ uClibc.1/include/grp.h 2008-09-27 17:15:56.000000000 +0200
@@ -167,31 +167,34 @@ extern int fgetgrent_r (FILE *__restrict
# endif
#endif /* POSIX or reentrant */
-#ifdef __USE_BSD
+#if defined __USE_BSD || defined __USE_GNU
# define __need_size_t
# include <stddef.h>
-/* Set the group set for the current user to GROUPS (N of them). */
-extern int setgroups (size_t __n, __const __gid_t *__groups) __THROW;
-
-#if 0
/* Store at most *NGROUPS members of the group set for USER into
*GROUPS. Also include GROUP. The actual number of groups found is
returned in *NGROUPS. Return -1 if the if *NGROUPS is too small.
+ In all cases the actual number of groups is stored in *NGROUPS.
This function is not part of POSIX and therefore no official
cancellation point. But due to similarity with an POSIX interface
or due to the implementation it is a cancellation point and
therefore not marked with __THROW. */
extern int getgrouplist (__const char *__user, __gid_t __group,
__gid_t *__groups, int *__ngroups);
+
#endif
+#if defined __USE_BSD
+
+/* Set the group set for the current user to GROUPS (N of them). */
+extern int setgroups (size_t __n, __const __gid_t *__groups) __THROW;
+
/* Initialize the group set for the current user
by reading the group database and using all groups
of which USER is a member. Also include GROUP.
This function is not part of POSIX and therefore no official
diff -d -urpN -U5 uClibc.0/libc/pwd_grp/getgrouplist.c uClibc.1/libc/pwd_grp/getgrouplist.c
--- uClibc.0/libc/pwd_grp/getgrouplist.c 1970-01-01 01:00:00.000000000 +0100
+++ uClibc.1/libc/pwd_grp/getgrouplist.c 2008-09-28 00:34:50.000000000 +0200
@@ -0,0 +1,8 @@
+/*
+ * Copyright (C) 2000-2006 Erik Andersen <[EMAIL PROTECTED]>
+ *
+ * Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball.
+ */
+
+#define L_getgrouplist
+#include "pwd_grp.c"
diff -d -urpN -U5 uClibc.0/libc/pwd_grp/__getgrouplist_internal.c uClibc.1/libc/pwd_grp/__getgrouplist_internal.c
--- uClibc.0/libc/pwd_grp/__getgrouplist_internal.c 1970-01-01 01:00:00.000000000 +0100
+++ uClibc.1/libc/pwd_grp/__getgrouplist_internal.c 2008-09-28 00:34:50.000000000 +0200
@@ -0,0 +1,8 @@
+/*
+ * Copyright (C) 2000-2006 Erik Andersen <[EMAIL PROTECTED]>
+ *
+ * Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball.
+ */
+
+#define L___getgrouplist_internal
+#include "pwd_grp.c"
diff -d -urpN -U5 uClibc.0/libc/pwd_grp/pwd_grp.c uClibc.1/libc/pwd_grp/pwd_grp.c
--- uClibc.0/libc/pwd_grp/pwd_grp.c 2008-09-27 16:49:57.000000000 +0200
+++ uClibc.1/libc/pwd_grp/pwd_grp.c 2008-09-28 00:34:50.000000000 +0200
@@ -62,10 +62,12 @@ extern int __parsegrent(void *gr, char *
extern int __parsespent(void *sp, char *line) attribute_hidden;
extern int __pgsreader(int (*__parserfunc)(void *d, char *line), void *data,
char *__restrict line_buff, size_t buflen, FILE *f) attribute_hidden;
+extern gid_t* __getgrouplist_internal(const char *user, gid_t gid, int *ngroups) attribute_hidden;
+
/**********************************************************************/
/* For the various fget??ent_r funcs, return
*
* 0: success
* ENOENT: end-of-file encountered
@@ -682,66 +684,109 @@ struct spwd *sgetspent(const char *strin
return result;
}
#endif
/**********************************************************************/
-#ifdef L_initgroups
-
-#ifdef __USE_BSD
-
-libc_hidden_proto(setgroups)
+#ifdef L___getgrouplist_internal
-int initgroups(const char *user, gid_t gid)
+gid_t attribute_hidden *__getgrouplist_internal(const char *user, gid_t gid, int *ngroups)
{
FILE *grfile;
gid_t *group_list;
- int num_groups, rv;
- char **m;
+ int num_groups;
struct group group;
char buff[__UCLIBC_PWD_BUFFER_SIZE__];
- rv = -1;
+ *ngroups = num_groups = 1;
/* We alloc space for 8 gids at a time. */
- if (((group_list = (gid_t *) malloc(8*sizeof(gid_t *))) != NULL)
- && ((grfile = fopen(_PATH_GROUP, "r")) != NULL)
- ) {
+ group_list = malloc(8 * sizeof(group_list[0]));
+ if (!group_list)
+ return NULL;
- __STDIO_SET_USER_LOCKING(grfile);
+ group_list[0] = gid;
+ grfile = fopen(_PATH_GROUP, "r");
+ /* If /etc/group doesn't exist, we still return 1-element vector */
+ if (!grfile)
+ return group_list;
- *group_list = gid;
- num_groups = 1;
+ __STDIO_SET_USER_LOCKING(grfile);
- while (!__pgsreader(__parsegrent, &group, buff, sizeof(buff), grfile)) {
- assert(group.gr_mem); /* Must have at least a NULL terminator. */
- if (group.gr_gid != gid) {
- for (m=group.gr_mem ; *m ; m++) {
- if (!strcmp(*m, user)) {
- if (!(num_groups & 7)) {
- gid_t *tmp = (gid_t *)
- realloc(group_list,
- (num_groups+8) * sizeof(gid_t *));
- if (!tmp) {
- rv = -1;
- goto DO_CLOSE;
- }
- group_list = tmp;
- }
- group_list[num_groups++] = group.gr_gid;
- break;
- }
- }
+ while (!__pgsreader(__parsegrent, &group, buff, sizeof(buff), grfile)) {
+ char **m;
+
+ assert(group.gr_mem); /* Must have at least a NULL terminator. */
+ if (group.gr_gid == gid)
+ continue;
+ for (m = group.gr_mem; *m; m++) {
+ if (strcmp(*m, user) != 0)
+ continue;
+ if (!(num_groups & 7)) {
+ gid_t *tmp = realloc(group_list, (num_groups+8) * sizeof(group_list[0]));
+ if (!tmp)
+ goto DO_CLOSE;
+ group_list = tmp;
}
+ group_list[num_groups++] = group.gr_gid;
+ break;
}
+ }
- rv = setgroups(num_groups, group_list);
- DO_CLOSE:
- fclose(grfile);
+ DO_CLOSE:
+ fclose(grfile);
+ *ngroups = num_groups;
+ return group_list;
+}
+
+#endif
+/**********************************************************************/
+#ifdef L_getgrouplist
+
+#if defined __USE_BSD || defined __USE_GNU
+int getgrouplist(const char *user, gid_t gid, gid_t *groups, int *ngroups)
+{
+ int sz = *ngroups;
+ gid_t *group_list = __getgrouplist_internal(user, gid, ngroups);
+
+ if (!group_list) {
+ /* malloc failure - what shall we do?
+ * fail with ENOMEM? I bet users never check for that */
+ /* *ngroups = 1; - already done by __getgrouplist_internal */
+ if (sz) {
+ groups[0] = gid;
+ return 1;
+ }
+ return -1;
}
+ /* *ngroups is non-zero here */
- /* group_list will be NULL if initial malloc failed, which may trigger
- * warnings from various malloc debuggers. */
+ if (sz > *ngroups)
+ sz = *ngroups;
+ if (sz)
+ memcpy(groups, group_list, sz * sizeof(group_list[0]));
+ free(group_list);
+ if (sz < *ngroups)
+ return -1;
+ return sz;
+}
+#endif
+
+#endif
+/**********************************************************************/
+#ifdef L_initgroups
+
+#ifdef __USE_BSD
+libc_hidden_proto(setgroups)
+
+int initgroups(const char *user, gid_t gid)
+{
+ int rv;
+ int num_groups = ((unsigned)~0) >> 1; /* INT_MAX */
+ gid_t *group_list = __getgrouplist_internal(user, gid, &num_groups);
+ if (!group_list)
+ return -1;
+ rv = setgroups(num_groups, group_list);
free(group_list);
return rv;
}
#endif
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox
