useradd: Correctly set subuid/subgid when using -F by acdn-ndostert · Pull Request #1511 · shadow-maint/shadow

acdn-ndostert · 2026-01-17T09:50:19Z

I was not able to run the tests as described in doc/contributions/tests.md at the moment but I was able to build and test that useradd -F work as intended.

src/useradd.c

The -F flag should bypass the UID check and always add entries to the sub[ud]id while the -r flag should still be used in addition with the UID check. Closes: <shadow-maint#1255> Signed-off-by: ndostert <crawax@cwxlab.fr>

alejandro-colomar · 2026-01-18T14:14:25Z

src/useradd.c

 	    subuid_count > 0 && sub_uid_file_present () &&
-	    (!rflg || Fflg) &&
-	    (!user_id || (user_id <= uid_max && user_id >= uid_min));
+	    (!rflg && (!user_id || (user_id <= uid_max && user_id >= uid_min))) || Fflg;


This seems brittle. It is affected by the fact that && has more precedence than ||, which is something not well known.

Sorry I'm more a Python person, I can mostly read C code but not really write it.
Doesn't the parenthesis group the tests so there is not && and || in the same test ?

( !rflg && ( !user_id || ( user_id <= uid_max && user_id >= uid_min ) ) ) || Fflg;

Yes, but you have the full expression (see lines 2513 and above) as

a && b && && c && (...) || e

Those && and || don't necessarily interact as you'd expect.

Yes, but you have the full expression (see lines 2513 and above) as

a && b && && c && (...) || e

Those && and || don't necessarily interact as you'd expect.

Ah yes I see, well I'm out of my depth on that.
I will see if I can either "prove" this work or fix it.

And also, thanks for your help !

If I were you, I'd start from the original code, and do the minimal changes that achieve what you want. Don't try to make it look nice. The smaller the diff, the easier it will be to review. I can clean up the code afterwards to make it look good.

Github is eating my comments. Grr.

Please rebase. We've made it much easier to read and modify.

hallyn · 2026-01-26T02:43:51Z

Yeah, I think this is correct. But let's let it settle a bit longer, I'd like to take another look at the logic on a different day with a clear head.

acdn-ndostert · 2026-02-17T20:54:58Z

I think @alejandro-colomar is correct the fix is not right as the || operator bypass want_subuid_file (), subuid_count > 0 and sub_uid_file_present ()

I wrote this to test the operators:

#include <stdio.h>

static bool is_sub_uid = false;

int main() {

    // is_sub_uid = want_subuid_file () &&
    //     subuid_count > 0 && sub_uid_file_present () &&
    //     (!user_id || (user_id <= uid_max && user_id >= uid_min));

    // Case 1
    is_sub_uid = false &&
        false && false &&
        (false && (false || (false && false))) || true;

    if (is_sub_uid)
        printf("1\n");

    // Case 2
    is_sub_uid = false &&
        false && false &&
        ((false && (false || (false && false))) || true);

    if (is_sub_uid)
        printf("2\n");

    return 0;
}

And running it print 1
So my current solution (Case 1) is wrong and a possible fix is to add parenthesis (Case 2) so that Fflg only bypass rflg and the uid checks.

hallyn · 2026-02-18T16:07:13Z

Let's try adding the comment as not-a-reply.

I think it's clearest if you move the decision into a function. That leaves main easier to read, and gives you more space to clearly write out the decision to be made

diff --git a/src/useradd.c b/src/useradd.c
index b2fa8f79..a0c44aa3 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -2444,6 +2444,44 @@ static void check_uid_range(int rflg, uid_t user_id)
 	}
 
 }
+
+bool should_assign_subuid(void) {
+	uid_t uid_min = getdef_ulong ("UID_MIN", 1000UL);
+	uid_t uid_max = getdef_ulong ("UID_MAX", 60000UL);
+	unsigned long subuid_count = getdef_ulong ("SUB_UID_COUNT", 65536);
+
+	if (!want_subuid_file())
+		return false;
+	if (subuid_count <= 0)
+		return false;
+	if (!sub_uid_file_present())
+		return false;
+	if (rflg && !Fflg)
+		return false;
+	if (user_id > uid_max || user_id < uid_min)
+		return false;
+	return true;
+}
+
+bool should_assign_subgid(void) {
+	uid_t uid_min = getdef_ulong ("UID_MIN", 1000UL);
+	uid_t uid_max = getdef_ulong ("UID_MAX", 60000UL);
+	unsigned long subgid_count = getdef_ulong ("SUB_GID_COUNT", 65536);
+
+	if (!want_subgid_file())
+		return false;
+	if (subgid_count <= 0)
+		return false;
+	if (!sub_gid_file_present())
+		return false;
+	if (rflg && !Fflg)
+		return false;
+	if (user_id > uid_max || user_id < uid_min)
+		return false;
+	return true;
+}
+	unsigned long subgid_count = getdef_ulong ("SUB_GID_COUNT", 65536);
+
 /*
  * main - useradd command
  */
@@ -2493,18 +2531,8 @@ int main (int argc, char **argv)
 	process_selinux = !flags.chroot && !flags.prefix;
 
 #ifdef ENABLE_SUBIDS
-	uid_min = getdef_ulong ("UID_MIN", 1000UL);
-	uid_max = getdef_ulong ("UID_MAX", 60000UL);
-	subuid_count = getdef_ulong ("SUB_UID_COUNT", 65536);
-	subgid_count = getdef_ulong ("SUB_GID_COUNT", 65536);
-	is_sub_uid = want_subuid_file () &&
-	    subuid_count > 0 && sub_uid_file_present () &&
-	    (!rflg || Fflg) &&
-	    (!user_id || (user_id <= uid_max && user_id >= uid_min));
-	is_sub_gid = want_subgid_file() &&
-	    subgid_count > 0 && sub_gid_file_present() &&
-	    (!rflg || Fflg) &&
-	    (!user_id || (user_id <= uid_max && user_id >= uid_min));
+	is_sub_uid = should_assign_subuid();
+	is_sub_gid = should_assign_subgid();
 #endif				/* ENABLE_SUBIDS */
 
 	if (run_parts ("/etc/shadow-maint/useradd-pre.d", user_name,

alejandro-colomar · 2026-02-18T19:50:37Z

Let's try adding the comment as not-a-reply.

I think it's clearest if you move the decision into a function. That leaves main easier to read, and gives you more space to clearly write out the decision to be made

diff --git a/src/useradd.c b/src/useradd.c
index b2fa8f79..a0c44aa3 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -2444,6 +2444,44 @@ static void check_uid_range(int rflg, uid_t user_id)
 	}
 
 }
+
+bool should_assign_subuid(void) {
+	uid_t uid_min = getdef_ulong ("UID_MIN", 1000UL);
+	uid_t uid_max = getdef_ulong ("UID_MAX", 60000UL);
+	unsigned long subuid_count = getdef_ulong ("SUB_UID_COUNT", 65536);
+
+	if (!want_subuid_file())
+		return false;
+	if (subuid_count <= 0)
+		return false;
+	if (!sub_uid_file_present())
+		return false;
+	if (rflg && !Fflg)
+		return false;
+	if (user_id > uid_max || user_id < uid_min)
+		return false;
+	return true;
+}
+
+bool should_assign_subgid(void) {
+	uid_t uid_min = getdef_ulong ("UID_MIN", 1000UL);
+	uid_t uid_max = getdef_ulong ("UID_MAX", 60000UL);
+	unsigned long subgid_count = getdef_ulong ("SUB_GID_COUNT", 65536);
+
+	if (!want_subgid_file())
+		return false;
+	if (subgid_count <= 0)
+		return false;
+	if (!sub_gid_file_present())
+		return false;
+	if (rflg && !Fflg)
+		return false;
+	if (user_id > uid_max || user_id < uid_min)
+		return false;
+	return true;
+}
+	unsigned long subgid_count = getdef_ulong ("SUB_GID_COUNT", 65536);
+
 /*
  * main - useradd command
  */
@@ -2493,18 +2531,8 @@ int main (int argc, char **argv)
 	process_selinux = !flags.chroot && !flags.prefix;
 
 #ifdef ENABLE_SUBIDS
-	uid_min = getdef_ulong ("UID_MIN", 1000UL);
-	uid_max = getdef_ulong ("UID_MAX", 60000UL);
-	subuid_count = getdef_ulong ("SUB_UID_COUNT", 65536);
-	subgid_count = getdef_ulong ("SUB_GID_COUNT", 65536);
-	is_sub_uid = want_subuid_file () &&
-	    subuid_count > 0 && sub_uid_file_present () &&
-	    (!rflg || Fflg) &&
-	    (!user_id || (user_id <= uid_max && user_id >= uid_min));
-	is_sub_gid = want_subgid_file() &&
-	    subgid_count > 0 && sub_gid_file_present() &&
-	    (!rflg || Fflg) &&
-	    (!user_id || (user_id <= uid_max && user_id >= uid_min));
+	is_sub_uid = should_assign_subuid();
+	is_sub_gid = should_assign_subgid();
 #endif				/* ENABLE_SUBIDS */
 
 	if (run_parts ("/etc/shadow-maint/useradd-pre.d", user_name,

Agree. I will publish a PR that refactors the current code. We can merge that ASAP, so that @acdn-ndostert can then rebase, and do a minimal change to that function.

alejandro-colomar reviewed Jan 17, 2026

View reviewed changes

src/useradd.c Show resolved Hide resolved

acdn-ndostert force-pushed the useradd_subuid branch 2 times, most recently from 0066ad0 to 4ecf680 Compare January 17, 2026 17:19

alejandro-colomar requested review from hallyn and ikerexxe January 17, 2026 17:28

useradd: Correctly set subuid/subgid when using -F

1185c23

The -F flag should bypass the UID check and always add entries to the sub[ud]id while the -r flag should still be used in addition with the UID check. Closes: <shadow-maint#1255> Signed-off-by: ndostert <crawax@cwxlab.fr>

acdn-ndostert force-pushed the useradd_subuid branch from 4ecf680 to 1185c23 Compare January 18, 2026 07:42

alejandro-colomar reviewed Jan 18, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

useradd: Correctly set subuid/subgid when using -F#1511

useradd: Correctly set subuid/subgid when using -F#1511
acdn-ndostert wants to merge 1 commit intoshadow-maint:masterfrom
acdn-ndostert:useradd_subuid

acdn-ndostert commented Jan 17, 2026

Uh oh!

Uh oh!

alejandro-colomar Jan 18, 2026

Uh oh!

acdn-ndostert Jan 18, 2026

Uh oh!

alejandro-colomar Jan 18, 2026 •

edited

Loading

Uh oh!

acdn-ndostert Jan 20, 2026

Uh oh!

alejandro-colomar Feb 17, 2026

Uh oh!

hallyn Feb 18, 2026

Uh oh!

alejandro-colomar Feb 18, 2026

Uh oh!

hallyn commented Jan 26, 2026

Uh oh!

acdn-ndostert commented Feb 17, 2026

Uh oh!

hallyn commented Feb 18, 2026

Uh oh!

alejandro-colomar commented Feb 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

acdn-ndostert commented Jan 17, 2026

Uh oh!

Uh oh!

alejandro-colomar Jan 18, 2026

Choose a reason for hiding this comment

Uh oh!

acdn-ndostert Jan 18, 2026

Choose a reason for hiding this comment

Uh oh!

alejandro-colomar Jan 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acdn-ndostert Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

alejandro-colomar Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

hallyn Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

alejandro-colomar Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

hallyn commented Jan 26, 2026

Uh oh!

acdn-ndostert commented Feb 17, 2026

Uh oh!

hallyn commented Feb 18, 2026

Uh oh!

alejandro-colomar commented Feb 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alejandro-colomar Jan 18, 2026 •

edited

Loading