[PATCH] make id conform SUSv3 - bugfix and size reduction V5

[Denys Vlasenko]

On Wednesday 17 September 2008 01:22, Tito wrote:
> On Monday 15 September 2008 21:06:34 Tito wrote:
> > On Sunday 14 September 2008 21:35:57 you wrote:
> > > On Sunday 14 September 2008 19:16, Tito wrote:
> > > > Hi,
> > > > sadly there was a bug in the previous patch:
> > > > 
> > > >        n = getgroups(0, 0);
> > > >        groups = (gid_t *)xmalloc(sizeof(gid_t) * n);
> > > >        getgroups(n, (gid_t *)groups);
> > > > 
> > > > This code works only for the current process but doesn't accept
> > > > an username so for example:
> > > > 
> > > > ./busybox id -G john
> > > > 
> > > > returns the groups of the owner of the running process and
> > > > not of user john. I wonder how could i've been so blind...
> > > > The current patch fixes this bug and does some shrinkage
> > > > so that the correct behaviour is not so exepensive in terms
> > > > of binary size. Bloat-o-meter says:
> > > > 
> > > > scripts/bloat-o-meter busybox_old busybox_unstripped
> > > > function                                             old     new   delta
> > > > print_group_list                                       -     155    +155
> > > > print_list_helper                                      -      81     +81
> > > > .rodata                                           119213  119214      +1
> > > > id_main                                              494     315    -179
> > > > ------------------------------------------------------------------------------
> > > > (add/remove: 2/0 grow/shrink: 1/1 up/down: 237/-179)           Total: 58 bytes
> > > > 
> > > > The easiest way to implement it would have been to use:
> > > > 
> > > >    int getgrouplist(const char *user, gid_t group, gid_t *groups, int *ngroups);
> > > > 
> > > > but man says:   This function is non-standard; it appears on most BSDs.
> > > > (needs _BSD_SOURCE to be defined) and busybox's libpwdgrp
> > > > doesn't know about it.
> > > > So I reinvented the wheel........ 
> > > 
> > > Please implement it as getgrouplist() in libpwdgrp,
> > > and then use getgrouplist() in id.c
> > > 
> > > I hope that most libc's have getgrouplist() as this seems to be
> > > the only semi-standard way to retrieve this information -
> > > I used it in my micro-nscd too. POSIX simply forgot to add
> > > a function which does this.
> > > 
> > > Also, svn has some changes in id.c, please rediff against svn.
> > > Thanks
> > > --
> > > vda
> > > 
> > 
> > Hi Denys,
> > rediffed against current svn and tested.
> > Please apply if you like it.
> > I will think about adding getgrouplist to libpwdgrp
> > later as i don't know if my skills are enough
> > to touch this voodoo stuff. 
> > IMHO for id this gid_t *list stuff is useless
> > as we don't need to store the id's but just
> > retrieve them and print them on thr fly....
> > maybe for other applets it could be useful....
> > 
> > Ciao,
> > Tito
> > 
> > PS:: added one more bugfix to patch V4.
> > 
> 
> Hi,
> added correct euid egid handling to be fully SuSv3 compliant.
> Diff is against current svn. Tested and seems to work
> like GNU id.
> Hints, improvements and critics are welcome!

+static short print_list_helper(const char *fmt, gid_t gid, const char* prefix, unsigned flags)

Use int as a return value instead of short,
usually this results in smaller code.


+static short print_group_list(uid_t uid, gid_t gid, unsigned flags)
+{
+	struct group *g;
+	char *line;
+	short status = EXIT_SUCCESS;
+	const char *user = bb_getpwuid(NULL, -1, uid);
+	FILE *group_file = xfopen_for_read(bb_path_group_file);
+
+	status |= print_list_helper("%s", gid, " groups=", flags);
+	if (gid != geteuid() && flags)
+		status |= print_list_helper(" %s", geteuid(), ",", flags);
+	while ((line = xmalloc_fgetline(group_file)) != NULL) {
...
+}

This is wrong. Who says that group information is in /etc/group AT ALL?
What if it's in a LDAP db and is accessed by NSS glibc hooks?

That's why you must use getgrouplist(). IOW, you must
use libc interfaces.


-			if (flags & JUST_USER) {
-				printf("%u\n", (unsigned)uid);
-			}
-			if (flags & JUST_GROUP) {
-				printf("%u\n", (unsigned)gid);
-			}
+			printf("%u\n", (flags & JUST_USER) ? uid : gid);

These seems to be a set of fixes unrelated to getgrouplist() thing.
Can you submit them separately?
--
vda