On Tuesday 03 February 2009 16:24, walter harms wrote:
> 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).Yes, confusingly, the functions with such a desriptive name is from SELinux itself! http://www.linuxmanpages.com/man3/is_selinux_enabled.3.php Who could have though it... we are so used to "chmod", "brk". I'd expect "isselinon" instead of "is_selinux_enabled" :) ;) > 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 I don't have much idea what SELinux needs. I just fulfil requests from SELinux users who (hopefully) know better. > >> 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 ? In these sad days of total political correctness we not only have to respect gays and lesbians, we need to accomodate devices in e.g. /tmp. What a cruel world. See attached. This is the login running on /tmp/tty12: # who USER TTY IDLE TIME HOST root tty2 01:08 Feb 3 21:48:11 root /tmp/tty12 ? Feb 3 22:53:40 > >> 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 ? Of course I check it: + pw = bb_getpwnam_r(username); + if (!pw) + goto auth_failed; is it wrong? > > + /* > > + 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 I am now even more confused. > > +/* > > + 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 ?! Am I disagreeing with that here? not at all. I mean that instead returning a value by storing it in *pw2, why can't just return it as a, well, return value of the function? And since pw has ->pw_name, you don't need writable ptr "char *username" and its attendant "int userlen", user of check_pswd should just inspect ->pw_name. What is has to do with _SC_LOGIN_NAME_MAX I don't know > > 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() ? Yes. It exec's inside. That frees everything. -- vda
getty_gay.tar.gz
Description: application/tgz
_______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
