more on id

[Tito]

On Saturday 27 September 2008 23:53:15 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.
> > 
> > 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.
> > 
> > > # [PATCH 3/3]  "euid and egid handling" (forgot the subject here  ;-) 
> > > http://www.busybox.net/lists/busybox/2008-September/033080.html
> > 
> > Patch 2 also mentions "+ fixes".
> > Can you send these fixes (dropping bb_getgrouplist_malloc() for now)?
> > 
> > Thanks.
> > --
> > vda
> > 
> 
> Hi Denys,
> I reworked id taking into account your objections.
> I'm sending a drop in replacement for the moment as
> I would like to hear your opinion about it before producing a diff.
> The pros are:
> 
> 1) the size increase is little:
>  scripts/bloat-o-meter busybox_old  busybox_unstripped
> function                                             old     new   delta
> print_single                                           -     129    +129
> print_group_list                                       -      84     +84
> .rodata                                           119055  119046      -9
> printf_full                                           44       -     -44
> id_main                                              539     393    -146
> ------------------------------------------------------------------------------
> (add/remove: 2/1 grow/shrink: 0/2 up/down: 213/-199)           Total: 14 bytes
> (BTW: can't say why  bb_getgrouplist_malloc doesn't show up in the list, maybe it is inlined?);
> 
> 2) the code allows for easy adding of euid and egid handling;
> 3) the code is more readable as there are less #ifdefs;
> 4) it returns EXIT_ERROR if a gid to groupname translation fails while printing a group list;
> 5) it returns EXIT_ERROR if getgrouplist is not available and we try to print a group list;
> 6) adding support for the groups command should be easy.  
> 
> The code is tested and it seems to work as expected but
> hints, critics and improvements are welcome.
> 
> Ciao,
> Tito
> 
> 
> 

Hi,
new version that fixes a bug  (that i introduced, not in svn).
 
Ciao,
Tito
-------------- next part --------------
A non-text attachment was scrubbed...
Name: id.c
Type: text/x-csrc
Size: 5324 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20081001/af98a457/attachment-0002.c