Skip to content

unixpwd: fix reentrance of unixpwd_setpwd() and unixpwd_setspw() #6729

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

unixpwd: fix reentrance of unixpwd_setpwd() and unixpwd_setspw() #6729

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5a1c50
unixpwd: fix reentrance of unixpwd_setpwd() and unixpwd_setspw()
semarie Oct 27, 2025
2a73b82
unixpwd: avoid double closing tmp descriptor on error
semarie Oct 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 34 additions & 11 deletions unixpwd/c/unixpwd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ unixpwd_setpwd(const char *user, char *password)
*pw;
char buf[BUFLEN];
int tmp;
FILE *tmp_file;
FILE *tmp_file,
*passwd_file;
char tmp_name[PATH_MAX];
struct stat statbuf;
int rc;
Expand All @@ -96,18 +97,28 @@ unixpwd_setpwd(const char *user, char *password)
tmp = mkstemp(tmp_name);
if (tmp == -1)
return errno;
if (stat(ETC_PASSWD, &statbuf) != 0

passwd_file = fopen(ETC_PASSWD, "rbe");
if (passwd_file == NULL) {
rc = errno;
close(tmp);
unlink(tmp_name);
return rc;
}

if (fstat(fileno(passwd_file), &statbuf) != 0
|| fchown(tmp, statbuf.st_uid, statbuf.st_gid) != 0
|| fchmod(tmp, statbuf.st_mode) != 0
|| (tmp_file = fdopen(tmp, "w")) == NULL) {
rc = errno ? errno : EPERM;
fclose(passwd_file);
close(tmp);
unlink(tmp_name);
return rc;
}

setpwent();
while (1) {
rc = getpwent_r(&pwd, buf, BUFLEN, &pw);
rc = fgetpwent_r(passwd_file, &pwd, buf, BUFLEN, &pw);
if (rc != 0 || !pw)
break;
if (!strcmp(user, pw->pw_name)) {
Expand All @@ -116,8 +127,8 @@ unixpwd_setpwd(const char *user, char *password)
}
putpwent(pw, tmp_file);
}
endpwent();

fclose(passwd_file);
fclose(tmp_file);
if (rc != ENOENT) {
unlink(tmp_name);
Expand All @@ -144,7 +155,8 @@ unixpwd_setspw(const char *user, char *password)
*sp;
char buf[BUFLEN];
int tmp;
FILE *tmp_file = NULL;
FILE *tmp_file = NULL,
*spasswd_file;
char tmp_name[PATH_MAX];
struct stat statbuf;
int rc;
Expand All @@ -154,26 +166,37 @@ unixpwd_setspw(const char *user, char *password)
tmp = mkstemp(tmp_name);
if (tmp == -1)
return errno;
if (stat(ETC_SPASSWD, &statbuf) != 0

spasswd_file = fopen(ETC_SPASSWD, "rbe");
if (spasswd_file == NULL) {
rc = errno;
close(tmp);
unlink(tmp_name);
return rc;
}

if (fstat(fileno(spasswd_file), &statbuf) != 0
|| fchown(tmp, statbuf.st_uid, statbuf.st_gid) != 0
|| fchmod(tmp, statbuf.st_mode) != 0
|| (tmp_file = fdopen(tmp, "w")) == NULL) {
rc = errno ? errno : EPERM;
fclose(spasswd_file);
close(tmp);
unlink(tmp_name);
return rc;
}
if (lckpwdf() != 0) {
fclose(spasswd_file);
if (tmp_file)
fclose(tmp_file);
close(tmp);
else
close(tmp);
unlink(tmp_name);
return ENOLCK;
}

setspent();
while (1) {
rc = getspent_r(&spw, buf, BUFLEN, &sp);
rc = fgetspent_r(spasswd_file, &spw, buf, BUFLEN, &sp);
if (rc != 0 || !sp)
break;
if (!strcmp(user, sp->sp_namp)) {
Expand All @@ -182,8 +205,8 @@ unixpwd_setspw(const char *user, char *password)
}
putspent(sp, tmp_file);
}
endspent();

fclose(spasswd_file);
fclose(tmp_file);
if (rc != ENOENT) {
ulckpwdf();
Copy link
Contributor

@edwintorok edwintorok Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is still global, right?
What happens if 2 threads try to use lckpwdf? Do only one of them get it, do both of them get it?
Could it result in a bug where there are 2 callers to this function from 2 threads, the 1st one obtains the lock with lckpwdf(), the 2nd one (noop) acquires the lock again, and then the 1st one releases the lock with ulckpwdf(), leaving the 2nd thread to operate on the file without holding any locks, potentially corrupting it.

(If lckpwdf() relies on file-locks, then those only work per-process, and the application needs to do its own locking with mutexes, i.e. we're back to where we started, to having a global lock around this whole function)

Copy link
Collaborator

@freddy77 freddy77 Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If 2 threads can obtain the lock at the same time it would be a very weird lock.
Yes, Glibc uses a file descriptor and a file lock so 2 threads in the same process will obtain the lock. How these functions are classified as multithread-safe I don't know...
BTW, the lock (if implemented, in MUSL they are empty stubs) should be global system wide, so it could block other processes.

Copy link
Collaborator

@freddy77 freddy77 Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About Ocaml lock, yes, it should be released in unixpwd/c/unixpwd_stubs.c.

Copy link
Contributor

@edwintorok edwintorok Oct 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but then we need to introduce another mutex here (which could still be beneficial, because it'd allow other OCaml threads to keep running, while this one deals with the password database, which is probably quite rare)

Copy link
Contributor

@edwintorok edwintorok Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although if the intention is just to get this to compile with Musl (#6729 (comment)), then keeping the OCaml runtime lock as is might be simpler (we don't gain any additional concurrency, and we don't risk introducing any concurrency bugs in our existing code either).

Expand Down
Loading