Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dead code #1041

Open
alejandro-colomar opened this issue Jul 4, 2024 · 2 comments
Open

Dead code #1041

alejandro-colomar opened this issue Jul 4, 2024 · 2 comments

Comments

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Jul 4, 2024

shadow/lib/valid.c

Lines 41 to 57 in 53ea42e

if ((NULL != ent->pw_name) && ('\0' == ent->pw_passwd[0])) {
if ('\0' == password[0]) {
return true; /* user entered nothing */
} else {
return false; /* user entered something! */
}
}
/*
* If there is no entry then we need a salt to use.
*/
if ((NULL == ent->pw_name) || ('\0' == ent->pw_passwd[0])) {
salt = "xx";
} else {
salt = ent->pw_passwd;
}

The only way to arrive at line 48 with

('\0' == ent->pw_passwd[0])

is that

(NULL == ent->pw_name)

But then, it means that the second part of the condition in line 53 is redundant, and can be removed.

@hallyn
Copy link
Member

hallyn commented Jul 18, 2024

Hm, note it's (now, at least) only used in sulogin (single user login).

I think the second part of

if ((NULL == ent->pw_name) || ('\0' == ent->pw_passwd[0])) {

is actually the important part. The first part is there to
guard the second check: If there was no entry, then
ent->pw_passwd is NULL, so we can't check ent->pw_passwd[0].

It might be more intuitive if it was

if ((NULL == ent->pw_passwd) || ('\0' == ent->pw_passwd[0])) {

But I'm not sure if there is some corner case that would fail -
perhaps pw_passwd would end up pointing at garbage from a previous
call, while pw->pw_name is reliably NULLed.

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Jul 18, 2024

Yeah, I think something is obscure there, and I prefer to not touch it. I fear that we'll break it if we touch it. I think let's keep this issue open, and forget about it, until someone has a deep look at the logic of that program.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants