login.c fun

[Denis Vlasenko]

Small voyage across login.c code.

Code per se is not that bad (I've seen much worse)
and applet adds only 4k. However reviewing/fixin it
was humerorus enough to share it with the list.

(Undoubtedly in the process of fixing it I possibly
added my own bugs...)


	tmp = ttyname(0);
	if (tmp && (strncmp(tmp, "/dev/", 5) == 0))
		safe_strncpy(tty, tmp + 5, sizeof(tty));
	else if (tmp && *tmp == '/')
		safe_strncpy(tty, tmp, sizeof(tty));
	else
		safe_strncpy(tty, "UNKNOWN", sizeof(tty));
....
	if (*tty != '/')
		snprintf(full_tty, sizeof(full_tty)-1, "/dev/%s", tty);
	else
		safe_strncpy(full_tty, tty, sizeof(full_tty)-1);
...

This code is positively masochistic. First it _strips_ "/dev/",
and they painfully tries to _guess it back_...

Also it seems to be capable of producing /dev/UNKNOWN (!!!)
if things will go really bad...

static void checkutmp(int picky)
{
...
		line = ttyname(0);
		if (!line) {
			puts(NO_TTY);
			exit(1);
		}
		if (strncmp(line, "/dev/", 5) == 0)

How many ttyname(0)'s do we need in single 4k application?


#define	NO_UTENT \
	"No utmp entry.  You must exec \"login\" from the lowest level \"sh\""
#define	NO_TTY \
	"Unable to determine your tty name."

Wonderful #defines. For one, they are used just once, nearby.
So what's the point in having them #defined?
Next, the text itself is is misleading, to say the least.
What "exec from the 'lowest' (what's that?) sh (eh??)"
User may be rightly puzzled why login needs to
"determine your tty" (especially considering
that message is lying. It appears not when there is
a problem with ttyname(), but when login thinks
that it sees malice: fstat(0) != stat(ttyname(0)).


Now can I bring your attention to the following gem?

static void setutmp(const char *name, const char *line ATTRIBUTE_UNUSED)
{
	time_t t_tmp = (time_t)utent.ut_time;

Cast is not needed, compiler will do it anyway. But it's
the least of the problems here...

	utent.ut_type = USER_PROCESS;
	strncpy(utent.ut_user, name, sizeof(utent.ut_user));
	time(&t_tmp);

Hello? We just wrote current time over the value which we got
from utent.ut_time! ??

	/* other fields already filled in by checkutmp above */
	setutent();
	pututline(&utent);
	endutent();
#ifdef CONFIG_FEATURE_WTMP
	if (access(bb_path_wtmp_file, R_OK|W_OK) == -1) {
		close(creat(bb_path_wtmp_file, 0664));
	}
	updwtmp(bb_path_wtmp_file, &utent);
#endif
}

WTF??
We stored data _twice_ into t_tmp, and never read it back. _Ever_.
t_tmp variable was not used for anything at all.

The same hair-raising story repeats in checkutmp():

static void checkutmp(int picky)
{
	char *line;
	struct utmp *ut;
...
		memset(&utent, 0, sizeof(utent));
		utent.ut_type = LOGIN_PROCESS;
		utent.ut_pid = pid;
		strncpy(utent.ut_line, line, sizeof(utent.ut_line));
		/* XXX - assumes /dev/tty?? */
		strncpy(utent.ut_id, utent.ut_line + 3, sizeof(utent.ut_id));
		strncpy(utent.ut_user, "LOGIN", sizeof(utent.ut_user));
		t_tmp = (time_t)utent.ut_time;
		time(&t_tmp);
	}
}

--
vda