Hi Miod,
Miod Vallat wrote on Sat, Oct 04, 2014 at 12:05:48PM +:
groupdel(8)
That utility is grossly disgusting. It's a pity we can't delete
it completely - we have no useable replacement for groupinfo(8).
All the other group(8) commands could, imho, be tedu'ed...
With respect to disgusting: The file user.c contains *four*
static functions containing more or less identical code: creategid(),
modify_gid(), append_group(), and rm_user_from_groups(), which you
are touching. Note the nice, consistent naming, by the way.
- creategid() uses fgetln(3), does no checking whatsoever, and
just leaves all existing lines as they are, even completely
broken ones. By the way, there are exactly two calls to
creategid(), and the group argument is always empty.
- modify_gid() uses fgets(3) with a LINE_MAX (2048) buffer - a limit
that is neither documented in group(5) nor in groupmod(8), and
expecting that people are aware of
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_397
makes little sense to me - it *deletes* the long line and continues
with the next line. It also checks that each line contains at least
one colon and again deletes offending lines. As an exception, it
allows the special entry +. No further checks are done.
- append_group() again uses fgets(3), again deletes long lines,
again deletes lines without any colon - *NOT* even allowing + -,
deletes trailing whitespace from lines the user will be added to
(surprise! new feature), then has this terrific code:
buf[(j = cc)] = '\0';
if (buf[strlen(buf) - 1] != ':')
strlcat(buf, ,, sizeof(buf));
cc = strlcat(buf, user, sizeof(buf)) + 1;
if (cc = sizeof(buf))
/* error handling */
buf[cc - 1] = '\n';
buf[cc] = '\0';
I mean, just in case strlen(buf) isn't = j = cc, let's do another
call to strlen(3). But what if strlen(buf) is LINE_MAX-1? Does
that result in silent omission of the comma, clobbering two user
names together? No it doesn't, because the code would have errored
out before with line too long, so the strlcat is obfuscation
and should be replaced by a mere buf[cc++] = ','; buf[cc] = '\0',
so the first three lines become just
j = cc;
if (buf[cc - 1] != ':')
buf[cc++] = ',';
buf[cc] = '\0';
Surprisingly, the + 1 before the overflow check is actually
correct, though
cc = strlcat(buf, user, sizeof(buf));
if (cc + 1 = sizeof(buf))
/* error handling */
buf[cc++] = '\n';
buf[cc] = '\0';
would no doubt be clearer.
- rm_user_from_groups() uses fgets(3), too, again deleting long lines,
even those that would become short enough by removing the user.
But it does much stricter error checking than all the other
functions. It requires exactly three colons - as Miod observed,
not allowing +.
So error checking is different in each of the functions.
I did not even check whether the duplicate parts of the code -
dozens of lines in each of the functions - are identical or
maybe contain subtle differences.
Do people run this crap as root? I wonder. I certainly don't.
If anybody cares, anybody should clean it up. Otherwise, i volunteer
to delete all the parts that require write access and just leave
those that merely need read access, i.e. don't need to run as root.
I certainly don't volunteer to do the required cleanup.
I'm not surprised the code is so shitty. I heard a talk by the
author, Alistair G. Crooks, at EuroBSDCon in Karlsruhe, and it
was about as bad as the code.
will complain (and delete) lines which do not have exactly
three colon characters in them. This is annoying in yp environments
where the last line of /etc/group is usually a single `+' character,
which is equivalent to `+:*::'
Invoking groupdel when /etc/group contains a final `+' line yields:
# groupdel foo
Groupdel? Cannot reproduce. That's not broken in this way, but
certainly in some other ways. I guess you mean userdel, somehow
your cut paste failed, i guess:
userdel: Malformed entry `+'. Skipping
and the `+' line disappears.
The following diff attempts to recognize and preserve such a line.
Your diff correctly applies lipstick to the lower lip of the pig,
forgetting about the upper lip, though (append_group()) - and about
all the turds at the other end of the pig, of course.
So this only helps with userdel(8), and with usermod(8) -S '' if
and only if the user is removed from all groups. With usermod(8)
-S somegroups, the failure mode is shifted to append_group() if the
user is left in somegroups.
That said, OK schwarze@ for either your version of the diff,
or the modified one below, which is slightly simpler and
has the same effect.
Yours,
Ingo
Index: user.c
===
RCS file:
