From 43a759d9f963b58632b2836cd93d98bab9183217 Mon Sep 17 00:00:00 2001 From: Tobias Deiminger Date: Wed, 6 May 2026 14:39:39 +0200 Subject: [PATCH 1/4] userdel: fix user busy detection for threads On Linux, userdel/usermod check all /proc/ status files to ensure a to-be-modified user has no more running tasks, or abort modification otherwise. However, the check failed to detect threads running as the user if the corresponding main thread ran as a different user. The user is deleted despite still being busy. This is due to passing a wrong value to check_status. The caller passed "/task", rather than "/task/". In consequence check_status tried to open "/proc//task/status" - a wrong path that never exists - open fails, and check_status always returns 0. The correct status file name would have been "/proc//task//status" instead. The bug can only be reproduced by rather exotic code using raw syscalls. POSIX does not allow threads to have different UIDs. To fix it, construct the correct path to the tid status file in the caller, before passing it to check_status. Reproducer: // setuid_thread.c #include #include #include #include #include #include static uid_t target_uid; static void *user_thread(void *arg) { syscall(SYS_setuid, (long)target_uid); for (;;) { printf("thread running as uid %d (pid=%d)\n", (int)target_uid, (int)getpid()); sleep(5); } return NULL; } int main(int argc, char *argv[]) { if (argc < 2) { fprintf(stderr, "Usage: %s \n", argv[0]); return 1; } struct passwd *pw = getpwnam(argv[1]); if (!pw) { fprintf(stderr, "user not found: %s\n", argv[1]); return 1; } target_uid = pw->pw_uid; pthread_t tid; pthread_create(&tid, NULL, user_thread, NULL); sleep(60); return 0; } Execute in a shell gcc setuid_thread.c -o setuid_thread sudo useradd --no-create-home testuser sudo ./setuid_thread testuser & sudo userdel testuser Behavior without fix: No output, testuser is deleted. Behavior with fix: Output "userdel: user testuser is currently used by process 178863". testuser is not deleted. Signed-off-by: Tobias Deiminger --- lib/user_busy.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/user_busy.c b/lib/user_busy.c index c2a1829f8e..dc85204f9d 100644 --- a/lib/user_busy.c +++ b/lib/user_busy.c @@ -241,13 +241,17 @@ static int user_busy_processes (const char *name, uid_t uid) if (task_dir != NULL) { while (NULL != (ent = readdir(task_dir))) { pid_t tid; + /* 27: xxxxxxxxxx/task/xxxxxxxxxx + \0 */ + char tid_sname[27]; + if (get_pid(ent->d_name, &tid) == -1) { continue; } if (tid == pid) { continue; } - if (check_status (name, task_path+6, uid) != 0) { + stprintf_a(tid_sname, "%ld/task/%ld", (long) pid, (long) tid); + if (check_status (name, tid_sname, uid) != 0) { (void) closedir (proc); (void) closedir (task_dir); #ifdef ENABLE_SUBIDS @@ -272,4 +276,3 @@ static int user_busy_processes (const char *name, uid_t uid) return 0; } #endif /* __linux__ */ - From f013f63711626211a0846bd8679ee9daf58cdc20 Mon Sep 17 00:00:00 2001 From: Tobias Deiminger Date: Wed, 6 May 2026 14:39:39 +0200 Subject: [PATCH 2/4] lib: simplify check_status and different_namespace interface Change the interface of check_status and different_namespace to take pid and tid instead of a partially constructed path string. This is simpler and counters bugs like in the previous commit by design. Signed-off-by: Tobias Deiminger --- lib/user_busy.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/lib/user_busy.c b/lib/user_busy.c index dc85204f9d..30adf35af0 100644 --- a/lib/user_busy.c +++ b/lib/user_busy.c @@ -36,7 +36,7 @@ #ifdef __linux__ -static int check_status (const char *name, const char *sname, uid_t uid); +static int check_status (const char *name, uid_t uid, pid_t pid, pid_t tid); static int user_busy_processes (const char *name, uid_t uid); #else /* !__linux__ */ static int user_busy_utmp (const char *name); @@ -94,13 +94,13 @@ user_busy_utmp(const char *name) #ifdef __linux__ #ifdef ENABLE_SUBIDS #define in_parentuid_range(uid) ((uid) >= parentuid && (uid) < parentuid + range) -static int different_namespace (const char *sname) +static int different_namespace (pid_t pid, pid_t tid) { /* 41: /proc/xxxxxxxxxx/task/xxxxxxxxxx/ns/user + \0 */ char path[41]; char buf[512], buf2[512]; - stprintf_a(path, "/proc/%s/ns/user", sname); + stprintf_a(path, "/proc/%d/task/%d/ns/user", pid, tid); if (readlinknul_a(path, buf) == -1) return 0; @@ -116,14 +116,14 @@ static int different_namespace (const char *sname) #endif /* ENABLE_SUBIDS */ -static int check_status (const char *name, const char *sname, uid_t uid) +static int check_status (const char *name, uid_t uid, pid_t pid, pid_t tid) { /* 40: /proc/xxxxxxxxxx/task/xxxxxxxxxx/status + \0 */ char status[40]; char line[1024]; FILE *sfile; - stprintf_a(status, "/proc/%s/status", sname); + stprintf_a(status, "/proc/%d/task/%d/status", pid, tid); sfile = fopen (status, "r"); if (NULL == sfile) { @@ -144,7 +144,7 @@ static int check_status (const char *name, const char *sname, uid_t uid) return 1; } #ifdef ENABLE_SUBIDS - if ( different_namespace (sname) + if ( different_namespace (pid, tid) && ( have_sub_uids(name, ruid, 1) || have_sub_uids(name, euid, 1) || have_sub_uids(name, suid, 1)) @@ -225,7 +225,7 @@ static int user_busy_processes (const char *name, uid_t uid) continue; } - if (check_status (name, tmp_d_name, uid) != 0) { + if (check_status (name, uid, pid, pid) != 0) { (void) closedir (proc); #ifdef ENABLE_SUBIDS sub_uid_close(true); @@ -241,17 +241,13 @@ static int user_busy_processes (const char *name, uid_t uid) if (task_dir != NULL) { while (NULL != (ent = readdir(task_dir))) { pid_t tid; - /* 27: xxxxxxxxxx/task/xxxxxxxxxx + \0 */ - char tid_sname[27]; - if (get_pid(ent->d_name, &tid) == -1) { continue; } if (tid == pid) { continue; } - stprintf_a(tid_sname, "%ld/task/%ld", (long) pid, (long) tid); - if (check_status (name, tid_sname, uid) != 0) { + if (check_status (name, uid, pid, tid) != 0) { (void) closedir (proc); (void) closedir (task_dir); #ifdef ENABLE_SUBIDS From 38ec4152c055debe1f23b5117d5fa9cfeba881ea Mon Sep 17 00:00:00 2001 From: Tobias Deiminger Date: Thu, 7 May 2026 21:50:59 +0200 Subject: [PATCH 3/4] autogen.sh: turn format issues into compiler errors This protects against undefined behavior from wrongly used conversion specifiers. Note: snprintf unit test intentionally uses an empty format string to test, well, the empt format string. Thus override format-zero-length for it. Signed-off-by: Tobias Deiminger fix format error --- autogen.sh | 2 ++ tests/unit/Makefile.am | 1 + 2 files changed, 3 insertions(+) diff --git a/autogen.sh b/autogen.sh index cd7b3dd638..f1740d1663 100755 --- a/autogen.sh +++ b/autogen.sh @@ -4,8 +4,10 @@ autoreconf -v -f --install "$(dirname "$0")" || exit 1 CFLAGS="-O2" CFLAGS="$CFLAGS -Wall" +CFLAGS="$CFLAGS -Wformat=2" CFLAGS="$CFLAGS -Wextra" CFLAGS="$CFLAGS -Werror=discarded-qualifiers" +CFLAGS="$CFLAGS -Werror=format" CFLAGS="$CFLAGS -Werror=implicit-function-declaration" CFLAGS="$CFLAGS -Werror=implicit-int" CFLAGS="$CFLAGS -Werror=incompatible-pointer-types" diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index 74f6721107..f8ea5ca2c5 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -100,6 +100,7 @@ test_snprintf_SOURCES = \ $(NULL) test_snprintf_CFLAGS = \ $(AM_CFLAGS) \ + -Wno-format-zero-length \ $(NULL) test_snprintf_LDFLAGS = \ $(NULL) From 3d1b219537adf748d27331c9126b06f4617a3264 Mon Sep 17 00:00:00 2001 From: Tobias Deiminger Date: Thu, 7 May 2026 22:47:24 +0200 Subject: [PATCH 4/4] lib: apply linux specific pid_t format consistently Also change previously existing linux specific format for pid_t to %d. Signed-off-by: Tobias Deiminger --- lib/user_busy.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/user_busy.c b/lib/user_busy.c index 30adf35af0..11ced18889 100644 --- a/lib/user_busy.c +++ b/lib/user_busy.c @@ -216,7 +216,7 @@ static int user_busy_processes (const char *name, uid_t uid) } /* Check if the process is in our chroot */ - stprintf_a(root_path, "/proc/%lu/root", (unsigned long) pid); + stprintf_a(root_path, "/proc/%d/root", pid); if (stat (root_path, &sbroot_process) != 0) { continue; } @@ -236,7 +236,7 @@ static int user_busy_processes (const char *name, uid_t uid) return 1; } - stprintf_a(task_path, "/proc/%lu/task", (unsigned long) pid); + stprintf_a(task_path, "/proc/%d/task", pid); task_dir = opendir (task_path); if (task_dir != NULL) { while (NULL != (ent = readdir(task_dir))) {