bb login.c

[walter harms]

Denys Vlasenko schrieb:
> On Sunday 01 February 2009 20:47, walter harms wrote:
>> Denys Vlasenko schrieb:
>>> On Saturday 31 January 2009 18:37, walter harms wrote:
>>>> Hi Denis,
>>>> i have broken down my patch to login.c into several pieces (4).
>>>> Shall i send the diff or the .c ?
>>> Please send diffs
>> attached all patches should apply on top of each other. The patches are based
>> on the login.c i send to the list end of last year.
>>
>> v0-v1: basicly moves SELINUX and LOGIN_SCRIPTS out of the way
> 
> -#if ENABLE_SELINUX
> -       if (is_selinux_enabled()) {
> -               security_context_t old_tty_sid, new_tty_sid;
> +       USE_SELINUX(initselinux(username, full_tty, &user_sid));
> 
> -               if (get_default_context(username, NULL, &user_sid)) {
> 
> but then
> 
> +static void initselinux(char *username, char *full_tty,
> +                                               security_context_t * user_sid)
> +{
> +       security_context_t old_tty_sid, new_tty_sid;
> +
> +       if (get_default_context(username, NULL, user_sid)) {
> +               bb_error_msg_and_die("cannot get SID for %s", username);
> +       }
> 
> You lost "if (is_selinux_enabled()) ..." part
> 


investigated: i am not sure that is_selinux_enabled() i correct in the first place
libbb has a  selinux_or_die(void).
If the !ENABLE_SELINUX case is removed we could that add to every function and
be happy. It may also be helpful with some function inside selinux/*.c



> 
>> v1-v2: rework the get ttynam() part, move from array to pointer
> 
> +       full_tty = ttyname(STDIN_FILENO);
> +       if (!full_tty || strncmp(full_tty, "/dev/", 5)) {
> +               full_tty = (char *) "UNKNOWN";
> +               short_tty = full_tty;
> +       } else {
> +               short_tty = full_tty + 5;
> 
> If ttyname does not start with "/dev/", you set it to "UNKNOWN".
> This is wrong.
> 

are there really cases where a tty is not inside /dev ?
and why not report UNKNOWN then ?


>> v2-v3: move pswd check for PAM and !PAM into check_pswd()
>> 	- fix getpwnam_r buffer size
> 
> +static
> +struct passwd *bb_getpwnam_r(char *username)
> +{
> +       struct passwd *pwdstruct,*ptr;
> +       struct passwd *pwd1;
> +       size_t len = 1024;
> +       long int initlen = sysconf(_SC_GETPW_R_SIZE_MAX);
> +
> +       if (initlen > 0)
> +               len = (size_t) initlen;
> 
> what if sysconf() return a value which doesn't fit in size_t?
> 
> +
> +       /*
> +          pwdstruct is a list of pointers to
> 
> no, it isn't
> 
> +          data stored in pwdbuf
> 
> stored where?
> 
> +        */
> +
> +       pwdstruct= xmalloc(len+sizeof(*pwdstruct));
> +       ptr=pwdstruct+len;
> 
> Bug. pwdstruct + n moves ptr n * sizeof(struct passwd) bytes!
> 

Investigated:
yes but not deadly. the realy bug is that len should be sizeof(*pwdstruct) :)
the bug occured when i changed char * for struct passwd *

in your patch you do check the return value of getpwnam_r(), intentionally ?

> 
> +       /*
> +         pwd1 will become NULL on not found
> +          getpwnam_r will become 0 on eror
> 
> getpwnam does not "become", it "returns".
> 
	and write error with 2*r

> 
> +/*
> +  this is a special case
> +*/
> +       if (!pamuser || !pamuser[0])
> +               goto auth_failed;
> 
> Does this comment actually explain something? If it says
> 'NULL or "" username is considered a failure', well,
> that's obvious from the code.
> 
> 
>         if (option_mask32 & LOGIN_OPT_f)
> ...
> ...
> ...
>                 opt &= ~LOGIN_OPT_f;
> 
> You forgot to copy updated flags to option_mask32.
> 
> 
> 
> I think check_pswd() can be better implemented as:
> 
> struct passwd *check_pswd(const char *username)
> 
> instead of
> 
> static int check_pswd(char *username, int userlen, struct passwd **pw2)

the username stuff has a problem the size should not be hardcode but discovered
by _SC_LOGIN_NAME_MAX


> 
> 
> When these issues are addressed:
> 
> function                                             old     new   delta
> login_main                                          1552    1584     +32
> 
> :(
> 
> I applied patches 1 and 2, thanks! Please look at attached patch.
> It's a cleaned up version of patch 3. I hesitate to apply it
> to svn, there might be more unexploded ordnance there.
> 
> If it would be at least not bigger with the patch according
> to "make bloatcheck", then maybe...

the "real" savings occur when you can think about the rest independently.

i noticed the only pw->pw_shell is needed in calling run_shell().
Does run_shell free buffers allocated by login() ?


re,
 wh

> --
> vda