mdo(1): Added extensive group setting options and --print-rule by thesynthax · Pull Request #3 · OlCe2/freebsd-src

thesynthax · 2025-07-15T12:33:34Z

Added options to:

set primary group (-g)
override supplementary groups (-G)
add, remove or reset supplementary groups (-s [+,-,@])
override Real UID (-U), saved UID (-R), effective UID (-E), real GID (-P), saved GID (-Q)
print the actual transition rules that will occur (--print-rule)

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

…oups Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

Multiple issues existed within the powerpc FP/VSX save/restore functionality, leading to register corruption and loss of register contents in specific scenarios involving high signal load and use of both floating point and VSX instructions. Issue #1 On little endian systems the PCB used the wrong location for the shadowed FP register within the larger VSX register. This appears to have been an attempt to correct issue #2 without understanding how the vector load/store instructions actually operate. Issue #2 On little endian systems, the VSX state save/restore routines swapped 32-bit words within the 64-bit aliased double word for the associated floating point register. This was due to the use of a word-oriented load/store vs. doubleword oriented load/store. Issue #3 The FPU was turned off in the PCB but not in hardware, leading to a potential race condition if the same thread was scheduled immediately after sigreturn. The triggering codebase for this is Go, which makes heavy use of signals and and generates an unusual mix of floating point and VSX assembler. As a result, when combined with th powerpc lazy FPU restore, a condition was repeatedly hit whereby the thread was interrupted in FP+VSX mode, then restored in FP only mode, thus reliably triggering the issues above. Also clean up the associated asm() style issue flagged by GitHub Actions. Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com> MFC after: 1 week Pull Request: freebsd#1756

OlCe2

This generally looks good. Please see inline comments.

usr.bin/mdo/mdo.c

OlCe2 · 2025-07-24T10:17:00Z

usr.bin/mdo/mdo.c

+			err(EXIT_FAILURE, "strdup failed");
+		char *tok = strtok(s, ",");
+		while (tok) {
+			if (strcmp(tok, "@") == 0) {


I think it would be better if @ is accepted only as the first token. Else, there is confusion between whether @ should reset everything first even when not in first position, or should reset groups after the first directives, which anyway would be silly (and I don't see a use case for this feature with automatically produced command lines).

usr.bin/mdo/mdo.c

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

OlCe2

Oh sorry, had not validated this GitHub's review, so perhaps some my comments from yesterday were not visible.

usr.bin/mdo/mdo.c

OlCe2

Having a set of tests would be great.

As a separate, subsequent commit, it would be nice to add some way to say that the user should not be changed, but only groups should. E.g., some new -k option? Ideally, I would have preferred that this behavior is triggered simply by not specifying -u, but since all credentials-changing program actually interpret not specifying a user as a request to change to root, diverging from that would be too surprising to users.

usr.bin/mdo/mdo.c

OlCe2 · 2025-07-29T09:24:55Z

usr.bin/mdo/mdo.c

+		if (pw != NULL) {
+			const long max = sysconf(_SC_NGROUPS_MAX) + 2;
+			base = malloc(sizeof(*base) * max);
+			if (!base)
+				err(EXIT_FAILURE, "malloc failed");
+			base_count = max;
+			getgrouplist(pw->pw_name, pw->pw_gid, base, &base_count);
+		}


As expressed in the comment just above, this code should be moved up in another block. At this point, you should only be processing supp_add and supp_rem.

-s option is having problems when I do that. I'm not able to add a new group for some reason.

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

OlCe2

General style problems:

Always enclose return values following a return in parenthesis.
Use _ to separate words in a variable name.

Generally, in everything you do, be consistent.

The changes I'm requesting here are for readability but also to fix some remaining bugs, notably that -k with -u doesn't trigger an error, that groups to remove are not obeyed when -G is passed, and switching to root without -u is not handled correctly.

usr.bin/mdo/mdo.c

OlCe2 · 2025-08-25T10:59:01Z

usr.bin/mdo/mdo.c

+	if (keepuser && (ruid_str != NULL || svuid_str != NULL || euid_str != NULL))
+		errx(EXIT_FAILURE, "-k and --ruid/--svuid/--euid cannot be used together");


These lines do not need any ID processing. Move them up before if (!keepuser), and add username_provided to the disjunction (else, -k will simply be ignored with -u, without printing an error message).

OlCe2 · 2025-08-25T13:49:59Z

usr.bin/mdo/mdo.c

+		int ngroups = getgroups(0, NULL);
+		gid_t *groups  = NULL;


All code with groups and ngroups here can be removed, as the current supplementary groups are irrelevant for a rule's "target" part. And, indeed, groups is not really used anywhere.

OlCe2 · 2025-08-25T13:52:55Z

usr.bin/mdo/mdo.c

+	} else if (pw != NULL && !uidonly) {
+		gid = pw->pw_gid;
+		override_gid = true;
+	} else if (ruid_str != NULL || svuid_str != NULL || euid_str != NULL) {


Note: Setting GIDs with the effective GID is in fact not necessary if all 3 strings are present. That would save a call to getegid() in this case, but that's not really important.

OlCe2 · 2025-08-25T13:55:21Z

usr.bin/mdo/mdo.c

 	struct setcred wcred = SETCRED_INITIALIZER;
 	u_int setcred_flags = 0;
 	bool uidonly = false;
+	bool keepuser = false;


Suggested change

bool keepuser = false;

bool keep_user = false;

OlCe2 · 2025-08-25T13:55:36Z

usr.bin/mdo/mdo.c

+	const char *group_mod_str = NULL;
 	struct setcred wcred = SETCRED_INITIALIZER;
 	u_int setcred_flags = 0;
 	bool uidonly = false;


Suggested change

bool uidonly = false;

bool uid_only = false;

OlCe2 · 2025-08-25T14:02:40Z

usr.bin/mdo/mdo.c

-				    username);
-			pw = getpwuid(uid);
+	if (!keepuser) {
+		if (username_provided) {


Remove this if (username_provided). The code inside it must be executed unconditionally.

Removing this creates problems for explicit UID changes. I changed the check though to work allow the default use of mdo i.e. mdo without any options.

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

github-actions · 2025-09-01T12:18:13Z

Thank you for taking the time to contribute to FreeBSD!
There is an issue that needs to be fixed:

Missing Signed-off-by lines^080dd63

Please review CONTRIBUTING.md, then update and push your branch again.

OlCe2

Globally fine, there are a few bugs still.

I think we should slightly change the semantics of -i and -k, see inline comments.

OlCe2 · 2025-09-01T12:42:40Z

usr.bin/mdo/mdo.c

+	} else {
+		pw = getpwuid(geteuid());
 		if (pw == NULL)
-			err(EXIT_FAILURE, "invalid username '%s'", username);
+			err(EXIT_FAILURE, "cannot determine current user");
 	}


If keeping the current user, we do not want to infer the groups from the DB, even if an entry exists. That feels too much surprising.

If some admin really wants to reload the groups from the DB, we'll let him enter the user name explicitly.

So, replace the content of the else block with (please indent as appropriate):

start_from_current_groups = true;

Also, please add in this same else block a comment explaining that, on keep_user the baseline for groups are the current ones.

OlCe2 · 2025-09-01T13:21:49Z

usr.bin/mdo/mdo.c

+		"\n"
+		"Options:\n"
+		"  -u <user>       Target user (name or UID)\n"
+		"  -i              Only change UID, skip groups\n"


I think we should change slightly semantics here:

Suggested change

" -i Only change UID, skip groups\n"

" -i Keep current groups, unless explicitly overridden\n"

This is still compatible with the initial mdo behavior, but a different extension than the one we have been doing so far. This new proposal has an important advantage: It allows more freedom to "admins" in that they can specify that mdo should leave groups alone unless explicitly told too (the previous behavior is to just leave groups alone entirely, and you would have to explicitly specify them all to their current values as the same starting point).

Basically, if the target user is in the password database, we just don't set the corresponding groups, and if it is not, we do not request -g (or the more fine-grained group options). But we let the admin still override groups manually if he wishes.

OlCe2 · 2025-09-01T13:31:49Z

usr.bin/mdo/mdo.c

 	struct setcred wcred = SETCRED_INITIALIZER;
 	u_int setcred_flags = 0;
-	bool uidonly = false;
+	bool uid_only = false;


Suggested change

bool uid_only = false;

bool start_from_current_groups = false;

In accordance with the change of semantics for -i.

OlCe2 · 2025-09-01T13:41:14Z

usr.bin/mdo/mdo.c

+			(euid_str == NULL && ruid_str == NULL && svuid_str == NULL)) {
+			uid_t uid = parse_user_pwd(username, &pw);
+
+			if (pw == NULL && primary_group == NULL && !uid_only)


This should rather be:

Suggested change

if (pw == NULL && primary_group == NULL && !uid_only)

if (!start_from_current_groups && pw == NULL &&

primary_group == NULL && (rgid_str == NULL || svgid_str == NULL || egid_str == NULL))

(please wrap the line to 80 columns and indent appropriately)

Please also move this if block out and just after the containing if (makes the general logic clearer, and changes the semantics in a way that makes some other check below unnecessary).

We don't need to treat the absence of explicitly specified supplementary groups as an error, if we effectively remove all supplementary groups in this case. If pw is not NULL, we readily have the group to use as a primary one, and we know we can call getgrouplist() to obtain the supplementary ones.

Please add a comment explaining the reason for this if (some elaboration of your own and perhaps a summary of the previous paragraph).

OlCe2 · 2025-09-01T17:03:54Z

usr.bin/mdo/mdo.c

+			uid_t uid = parse_user_pwd(username, &pw);
+
+			if (pw == NULL && primary_group == NULL && !uid_only)
+				errx(EXIT_FAILURE, "must specify -g when using a numeric UID");


Suggested change

errx(EXIT_FAILURE, "must specify -g when using a numeric UID");

errx(EXIT_FAILURE, "must specify primary groups or a user with an entry in the password database");

(please wrap the line as necessary; to be moved with its enclosing if as explained in my previous comment)

OlCe2 · 2025-09-01T21:05:31Z

usr.bin/mdo/mdo.c

+					}
+					free(groups);
+				}
+			}


Suggested change

}

OlCe2 · 2025-09-01T21:09:26Z

usr.bin/mdo/mdo.c

+				for (int i = 0; i < base_count; ++i) {
+					supp_groups_add = realloc_groups(supp_groups_add, add_count + 1);
+					supp_groups_add[add_count++] = groups[i];
+				}
+				free(groups);


Sorry, should have spotted this earlier, but this loop is in fact not necessary. Just set supp_groups_add to groups here.

OlCe2 · 2025-09-01T21:13:01Z

usr.bin/mdo/mdo.c

+			wcred.sc_supp_groups = NULL;
+			wcred.sc_supp_groups_nb = 0;
+			setcred_flags |= SETCREDF_SUPP_GROUPS;
+		} else {


Suggested change

} else {

The if in the else should be at the same level as the previous if.

OlCe2 · 2025-09-01T21:13:51Z

usr.bin/mdo/mdo.c

+			wcred.sc_supp_groups_nb = 0;
+			setcred_flags |= SETCREDF_SUPP_GROUPS;
+		} else {
+			if (pw != NULL && !uid_only) {


Suggested change

if (pw != NULL && !uid_only) {

if (!start_from_current_groups && pw != NULL) {

OlCe2 · 2025-09-01T21:18:33Z

usr.bin/mdo/mdo.c

+}
+
+static size_t
+remove_groups_from_array(gid_t *array, size_t count, const gid_t *remove_list, size_t remove_count)


Not a priority, but if you have some time: Replace by qsort() + duplicate removal on remove_list followed by a merge-like algorithm, since array is sorted and without duplicates.

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

Multiple issues existed within the powerpc FP/VSX save/restore functionality, leading to register corruption and loss of register contents in specific scenarios involving high signal load and use of both floating point and VSX instructions. Issue #1 On little endian systems the PCB used the wrong location for the shadowed FP register within the larger VSX register. This appears to have been an attempt to correct issue #2 without understanding how the vector load/store instructions actually operate. Issue #2 On little endian systems, the VSX state save/restore routines swapped 32-bit words within the 64-bit aliased double word for the associated floating point register. This was due to the use of a word-oriented load/store vs. doubleword oriented load/store. Issue #3 The FPU was turned off in the PCB but not in hardware, leading to a potential race condition if the same thread was scheduled immediately after sigreturn. The triggering codebase for this is Go, which makes heavy use of signals and and generates an unusual mix of floating point and VSX assembler. As a result, when combined with th powerpc lazy FPU restore, a condition was repeatedly hit whereby the thread was interrupted in FP+VSX mode, then restored in FP only mode, thus reliably triggering the issues above. Also clean up the associated asm() style issue flagged by GitHub Actions. Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com> MFC after: 1 week Pull Request: freebsd#1756 (cherry picked from commit 077e30e)

OlCe2 · 2025-11-21T10:03:44Z

Code here was largely rewritten, for lots of reason (have -G and -s work together, fix array reallocations, fix bugs related to getpwnam() using a single static storage, generally have options work together, in particular overriding ones and also including change of semantics for -i and -k, use algorithms linear in the number of groups).

The final result was committed as 3ca1e69, and will be present in 15.0.

OlCe2 · 2025-11-21T10:10:35Z

Actually, the --print-rule part was omitted as it had problems and wasn't high priority. To be reworked later.

thesynthax added 7 commits July 13, 2025 18:41

mdo(1): Added the -g and -G options for primary and supplementary groups

12f787c

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): -s option for addition, removal and reset of supplementary gr…

4ed7ff8

…oups Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): Supplementary groups duplication fix

0f4845f

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): Added explicit uid/gid overrides

e056514

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): CLI usage updates

de40081

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): added --print-rule

034c34e

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): fixed typo

400e22f

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

OlCe2 requested changes Jul 24, 2025

View reviewed changes

thesynthax added 3 commits July 27, 2025 08:52

mdo(1): Some style changes + improved condition checks

0cd24a3

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): fixed repetitive code

67c889f

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): --print-rule format fix

8ffb981

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

OlCe2 requested changes Jul 29, 2025

View reviewed changes

usr.bin/mdo/mdo.c Outdated Show resolved Hide resolved

usr.bin/mdo/mdo.c Outdated Show resolved Hide resolved

OlCe2 requested changes Jul 29, 2025

View reviewed changes

thesynthax added 4 commits August 12, 2025 09:30

mdo(1): Changes, logic reordering, error handling

f359981

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): supplementary groups fixed

f5cee13

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): added -k options for keep user, style changes

9e492d4

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): fixed explicit uid setting groups bug

10dcd6c

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

thesynthax force-pushed the mdo/groups branch from c7c9e8d to 10dcd6c Compare August 23, 2025 10:28

OlCe2 reviewed Aug 25, 2025

View reviewed changes

mdo(1): many changes

643d740

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

thesynthax changed the base branch from main to oc-thesynthax August 30, 2025 07:25

thesynthax added 4 commits August 30, 2025 11:13

mdo(1): mdo default use fixed

0adab1e

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): uid only transition fixed

e5572e9

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): style changes

f0e04de

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): style changes

080dd63

OlCe2 requested changes Sep 1, 2025

View reviewed changes

thesynthax added 2 commits September 2, 2025 10:22

mdo(1): change of semantics of -i and -k

60985b0

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): changed -k semantics

6258cab

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

thesynthax added 2 commits September 2, 2025 10:54

mdo(1): style

1ca0541

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

mdo(1): style

be30639

Signed-off-by: Kushagra Srivastava <kushagra1403@gmail.com>

OlCe2 closed this Nov 21, 2025

OlCe2 added the merged Was merged label Nov 21, 2025

OlCe2 self-assigned this Nov 21, 2025

		if (keepuser && (ruid_str != NULL \|\| svuid_str != NULL \|\| euid_str != NULL))
		errx(EXIT_FAILURE, "-k and --ruid/--svuid/--euid cannot be used together");

	" -i Only change UID, skip groups\n"
	" -i Keep current groups, unless explicitly overridden\n"

	bool uid_only = false;
	bool start_from_current_groups = false;

	if (pw == NULL && primary_group == NULL && !uid_only)
	if (!start_from_current_groups && pw == NULL &&
	primary_group == NULL && (rgid_str == NULL \|\| svgid_str == NULL \|\| egid_str == NULL))

	errx(EXIT_FAILURE, "must specify -g when using a numeric UID");
	errx(EXIT_FAILURE, "must specify primary groups or a user with an entry in the password database");

	if (pw != NULL && !uid_only) {
	if (!start_from_current_groups && pw != NULL) {

Conversation

thesynthax commented Jul 15, 2025

Uh oh!

OlCe2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OlCe2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OlCe2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlCe2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thesynthax Aug 29, 2025 • 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!

github-actions bot commented Sep 1, 2025

Uh oh!

OlCe2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thesynthax Aug 29, 2025 •

edited

Loading