[Toybox] [CLEANUP] su

[Rob Landley]

Sigh. I got badly derailed moving back from minnesota and starting a  
new day job, didn't I?


On 08/15/2013 07:01:56 PM, Strake wrote:
> On 14/08/2013, Rob Landley <rob at landley.net> wrote:
> > The root user is uid 0, which could be _named_ anything. (Crazy  
> people
> > say "wheel".) So we need a getwpuid(0) else getpwnam(name) and then  
> do
> > the shadow lookup based on that name.
> 
> I would likely write a function getspuid, but I'm not sure how you
> wish to do this, so I'll leave it for now.

I think it was this message that got me doing a general su cleanup,  
which has been pending for a while. Trying to get my modified commands  
cleaned up and checked in, here's my notes on this one:

su cleanups:

Tighten up help text. Group options, use tabs instead of spaces after
options.

whitespace: no space between function and open parentheses (that's for  
flow
control statements). Do the function protocols the way Dennis Ritchie
intended, with the curly bracket on the next line.

Inline "deny()" as a goto (it's error handling, ok to use gotos to  
collate
error handling at the end of the function) and just have it say "No."

I tend to say "!strcmp("-", *toys.optargs)" instead of
"strcmp ("-", toys.optargs[0]) == 0" not just because it's shorter, but
because that's two instances of 0 not saying anything. Treating a  
pointer
as an array of length zero strikes me as "not comfortable with  
pointers".
There's struct->member syntax instead of struct[0].member for example.  
We
don't say "int x = 4+0;" to declare an integer. (And then I try to be
consistent about usage, so I remove it even though it's not really  
hurting
anything because that's not what the rest of the code looks like.)

Thing about "blah & blah ? blah : blah": I parenthesize the (blah &  
blah)
because the priority of & vs && is nonobvious, so I just habitually
parenthesize boolean operations used near logic operations.

The (char *) {one, two, three} bit in exec is a nice trick, but I'd like
to collate all the if (FLAG_l) logic together into a single block, and a
stack allocation won't last past the closing curly bracket safely. (I
thought of declaring it unconditionally up in the start of the function,
but it hasn't done the getpwnam yet.)

I strongly suspect we've got enough users of if (!getpwnam())
error_exit() to just wrap it as xgetpwnam() these days. And there's two
similar ones in id.c, I might as well complete the set...

The error messages "can't find password" and "bad password format"...
untranslated error message strings make me nervous, and I'm also  
reluctant
to have a password checking program give more information than necessary
about what the problem is. (This is a balance between "easier to track"
and "sysadmin needs to be able to debug their config".) For a remote  
login
thing you don't want to say whether or not a user exists, but for local
it's a bit more relaxed...

For the can't find case I'm just going perror_msg("no '%s', name) and
letting it append an error if it has one. (The shadow password functions
are crap so they don't set an errno for the not found case, but the  
basic
message should cover it.) So ! caught by "not $", and it denies without
asking for a password first.

The LSB spec here is surprisingly useless. If you don't specify the  
user,
you get an "unspecified user". The environment is "modified in  
unspecified
ways". Thanks, Linux Foundation! You're _exactly_ what I've come to  
expect.

The man page is a lot more useful, it says:

1) log su attempts. (Leave that as a todo.)
2) Always reset $PATH and $IFS.
3) For --login, copy $TERM, $XTERM, $COLORTERM, $XAUTHORITY, blank rest.
4) If not -m or -p, set $HOME, $SHELL, $USER, $LOGNAME for new user

Sigh. Environment variables are always fun: if I do
   char *s = getenv("TERM");
   clearenv();
   setenv("TERM", s, 1);

Is that a use after free? (It isn't if $TERM was in the inherited  
environment block that's never freed, but if somebody previously did a  
setenv(TERM) before xexec() called our main function without an exec,  
then maybe? Sigh.)

Ok, assume it is, make a snapshot_env() that malloc's a "NAME=VALUE"  
string I can putenv(), and then bring back the char *blah[] =  
{snapshot_env(), snapshot_env()}; array population trick but confine  
the scope to its own block.

Right. The result passes a basic "does it work" smoketest, in the  
absence of any sort of test suite, call it done for now and promote  
from pending to lsb directory.

Rob