Skip to content

src/newgrp.c: sg(1): Various source code improvements and bug fixes #1461

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

Jump to bottom
Open
alejandro-colomar wants to merge 22 commits into
base: master
Choose a base branch
from
Open

src/newgrp.c: sg(1): Various source code improvements and bug fixes #1461

alejandro-colomar wants to merge 22 commits into from

Conversation

@alejandro-colomar
Copy link
Collaborator

@alejandro-colomar alejandro-colomar commented Dec 30, 2025 •
edited
Loading

Cc: @stoeckmann , @ikerexxe

Revisions:

v2
  • Unname argc.
$ git rd -U0
 1:  ef0420019 =  1:  ef0420019 src/newgrp.c: Document '-l'
 2:  b6d4f9b7a =  2:  b6d4f9b7a src/newgrp.c: sg(1): Refactor, to remove 'else' branch
 3:  56166b669 =  3:  56166b669 src/newgrp.c: sg(1): -c: Fail if the option is used without an argument
 4:  2b8316a3f =  4:  2b8316a3f src/newgrp.c: Split conditional block
 5:  8046815f8 !  5:  6843c19b3 src/newgrp.c: Use argv instead of argc to check an argument
    @@ Commit message
    +    Unname 'argc' to avoid diagnostics about the unused argument.
    +
    @@ src/newgrp.c: static void syslog_sg (const char *name, const char *group)
    -+main(int argc, char *argv[argc + 1])
    ++main(int, char *argv[])
 6:  17c01e5f4 !  6:  028e5c748 src/newgrp.c: Check '-l' and '-' in the documented order
    @@ src/newgrp.c
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
 7:  82c563b90 !  7:  3a0892de5 src/newgrp.c: newgrp(1): Remove 'else's after 'goto'
    @@ src/newgrp.c
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
    @@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
 8:  e7ee9d344 !  8:  4246adf9a src/newgrp.c: De-duplicate check
    @@ src/newgrp.c
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
    @@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
 9:  f58234eb3 !  9:  1f62687b7 src/newgrp.c: Separate code handling group name and code handling command
    @@ src/newgrp.c
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
    @@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
    @@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
10:  03bb4b4ef ! 10:  67834ed31 src/newgrp.c: Refactor conditionals to de-duplicate code
    @@ src/newgrp.c
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
    @@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
11:  5ae74951d ! 11:  f11890b4f src/newgrp.c: Fix indentation
    @@ src/newgrp.c
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
12:  de4cc76cc ! 12:  85706b5ec src/newgrp.c: Fail if there are trailing (unexpected) arguments
    @@ src/newgrp.c
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
13:  33dfdafe9 ! 13:  499eadeb2 src/newgrp.c: Remove incorrect comment
    @@ src/newgrp.c
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
14:  a9af91f67 ! 14:  3573e24a6 src/newgrp.c: sg(1): Use 'goto failure;' to fail
    @@ src/newgrp.c
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
15:  6b652d160 = 15:  d78ede7e7 lib/prototypes.h: Fix indentation
16:  99c712e35 ! 16:  89c89007c src/newgrp.c: Report SHADOW_AUDIT_FAILURE on failure
    @@ src/newgrp.c
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
17:  41fa8b9e8 ! 17:  657823c6f src/newgrp.c: Fix error handling
    @@ src/newgrp.c
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
18:  fc5e5462b = 18:  919b2c77d src/newgrp.c: syslog_sg(): Use Prog instead of its pattern
19:  23acca2c6 ! 19:  28aae3568 src/newgrp.c: Use idiomatic pointer arithmetic notation
    @@ src/newgrp.c
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
    @@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
    @@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
    @@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
20:  0e4f97b2a ! 20:  e4266db15 src/newgrp.c: Remove redundant comment
    @@ src/newgrp.c: static void check_perms (const struct group *grp,
    -@@ src/newgrp.c: main(int argc, char *argv[argc + 1])
    +@@ src/newgrp.c: main(int, char *argv[])
v3
  • Accept both -l and - in the same command, for consistency with su(1). [@stoeckmann ]
  • Remove redundant input validation. [@stoeckmann ]
$ git rd 
 1:  ef0420019 =  1:  ef0420019 src/newgrp.c: Document '-l'
 2:  b6d4f9b7a =  2:  b6d4f9b7a src/newgrp.c: sg(1): Refactor, to remove 'else' branch
 3:  56166b669 =  3:  56166b669 src/newgrp.c: sg(1): -c: Fail if the option is used without an argument
 4:  2b8316a3f =  4:  2b8316a3f src/newgrp.c: Split conditional block
 5:  6843c19b3 =  5:  6843c19b3 src/newgrp.c: Use argv instead of argc to check an argument
 6:  028e5c748 =  6:  028e5c748 src/newgrp.c: Check '-l' and '-' in the documented order
 7:  3a0892de5 =  7:  3a0892de5 src/newgrp.c: newgrp(1): Remove 'else's after 'goto'
 8:  4246adf9a =  8:  4246adf9a src/newgrp.c: De-duplicate check
 9:  1f62687b7 =  9:  1f62687b7 src/newgrp.c: Separate code handling group name and code handling command
10:  67834ed31 = 10:  67834ed31 src/newgrp.c: Refactor conditionals to de-duplicate code
11:  f11890b4f = 11:  f11890b4f src/newgrp.c: Fix indentation
12:  85706b5ec = 12:  85706b5ec src/newgrp.c: Fail if there are trailing (unexpected) arguments
13:  499eadeb2 = 13:  499eadeb2 src/newgrp.c: Remove incorrect comment
14:  3573e24a6 = 14:  3573e24a6 src/newgrp.c: sg(1): Use 'goto failure;' to fail
15:  d78ede7e7 = 15:  d78ede7e7 lib/prototypes.h: Fix indentation
16:  89c89007c = 16:  89c89007c src/newgrp.c: Report SHADOW_AUDIT_FAILURE on failure
17:  657823c6f = 17:  657823c6f src/newgrp.c: Fix error handling
18:  919b2c77d = 18:  919b2c77d src/newgrp.c: syslog_sg(): Use Prog instead of its pattern
19:  28aae3568 = 19:  28aae3568 src/newgrp.c: Use idiomatic pointer arithmetic notation
20:  e4266db15 = 20:  e4266db15 src/newgrp.c: Remove redundant comment
 -:  --------- > 21:  29c24fed6 src/newgrp.c: Accept both -l and - in the same command
 -:  --------- > 22:  df3bb4cbe src/newgrp.c: Remove redundant input validation

@alejandro-colomar
src/newgrp.c: Document '-l'
ef04200 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: sg(1): Refactor, to remove 'else' branch
b6d4f9b 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: sg(1): -c: Fail if the option is used without an argument
56166b6 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Split conditional block
2b8316a 
This way, the code is better self-documented, in the way that '-c' is
pretty much independent of the actual command, except that if it is
specified, a command is required, but is otherwise skipped when parsing
the command line.

It also reduces indentation, which is another way of saying it reduces
complexity of the code and thus improves readability.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar alejandro-colomar self-assigned this Dec 30, 2025
@alejandro-colomar alejandro-colomar changed the title src/newgrp.c: sg(1): Various source code improvements src/newgrp.c: sg(1): Various source code improvements and bug fixes Dec 30, 2025
@alejandro-colomar alejandro-colomar linked an issue Dec 30, 2025 that may be closed by this pull request
src/newgrp.c: Reporting success on failure #1462
Closed
@alejandro-colomar alejandro-colomar changed the title src/newgrp.c: sg(1): Various source code improvements and bug fixes src/newgrp.c: sg(1): Various (many) source code improvements and bug fixes Dec 30, 2025
@alejandro-colomar alejandro-colomar changed the title src/newgrp.c: sg(1): Various (many) source code improvements and bug fixes src/newgrp.c: sg(1): Various source code improvements and bug fixes Dec 30, 2025
stoeckmann

This comment was marked as resolved.

Sign in to view

@alejandro-colomar
src/newgrp.c: Use argv instead of argc to check an argument
6843c19 
It does the same, and avoids having to update both argc and argv, which
is prone to mistakes.

Unname 'argc' to avoid diagnostics about the unused argument.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Check '-l' and '-' in the documented order
028e5c7 
This makes the code more readable, by more closely matching its
documentation.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: newgrp(1): Remove 'else's after 'goto'
3a0892d 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: De-duplicate check
4246adf 
Both sg(1) and newgrp(1) reject any flags after the optional '-l'/'-'.
Do the check once in the common path.

Also, remove some comments that are getting less correct.

BTW, in one path we were doing the check with manual byte operations,
and in the other with strprefix().  Let's unify with neither of them;
strspn(3) is standard, and better (it allows multi-character search in
a single call, which we need elsewhere, which is why I'll unify the
entire project with strspn(3) in a future commit).

Also BTW, in the case of sg(1), we were exiting early, without
'goto failure;'.  I presume that was an accident.  The only difference
is the audit code, which I don't see a reason to skip.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Separate code handling group name and code handling com…
1f62687 
…mand

This will allow some deduplication of code in a following commit.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Refactor conditionals to de-duplicate code
67834ed 
Both sg(1) and newgrp(1) handle the group name in the same way if it
exists.  They differ in how they handle the absence of it.  Reflect that
in the source code.

Use a block to avoid changing the indentation.  This will make the diff
easier to review.  A future patch will clean up the indentation.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Fix indentation
f11890b 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Fail if there are trailing (unexpected) arguments
85706b5 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Remove incorrect comment
499eade 
This comment has several incorrections:

-  '-l'/'-' is now exclusive of newgrp(1); it works in sg(1) too.
-  The group name is mandatory in sg(1).
-  'Any remaining arguments' are not used to execute a command.
   Only the first one is used, and everything else was silently
   being ignored, until in last commit I made it fail hard.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: sg(1): Use 'goto failure;' to fail
3573e24 
Everywhere else we were failing that way.  I don't see any reason to
avoid the audit code here.  It probably was a mistake.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/prototypes.h: Fix indentation
d78ede7 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Report SHADOW_AUDIT_FAILURE on failure
89c8900 
Also, align the arguments more consistently with the call above.

Fixes: 8dfe21f (2025-03-03; "src/: update group audit messages")
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Fix error handling
657823c 
Use 'goto failure;', as we forgot to closelog(3).

Also, as always, remove the redundant nearby comment.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: syslog_sg(): Use Prog instead of its pattern
919b2c7 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Use idiomatic pointer arithmetic notation
28aae35 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Remove redundant comment
e4266db 
The usage() function already documents the syntax.  Let's keep a single
source of truth.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Accept both -l and - in the same command
29c24fe 
This is for consistency with su(1).

Reported-by: Tobias Stoeckmann <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/newgrp.c: Remove redundant input validation
df3bb4c 
is_valid_group_name() already fails if the name starts with '-'.
This changes the diagnostic message, but other than that it's fine.
Let's prefer the simpler code.

Reported-by: Tobias Stoeckmann <[email protected]>
Suggested-by: Tobias Stoeckmann <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hallyn hallyn Awaiting requested review from hallyn

@ikerexxe ikerexxe Awaiting requested review from ikerexxe

1 more reviewer

@stoeckmann stoeckmann stoeckmann left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@alejandro-colomar alejandro-colomar

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

src/newgrp.c: Reporting success on failure

2 participants

@alejandro-colomar @stoeckmann