Skip to content

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

Open
alejandro-colomar wants to merge 22 commits intoshadow-maint:masterfrom
alejandro-colomar:sg
Open

src/newgrp.c: sg(1): Various source code improvements and bug fixes#1461
alejandro-colomar wants to merge 22 commits intoshadow-maint:masterfrom
alejandro-colomar:sg

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
v3b
  • Rebase
$ git rd 
 1:  ef04200199d2 =  1:  5bc5e6157526 src/newgrp.c: Document '-l'
 2:  b6d4f9b7a5c8 =  2:  bf9e61c0ef7e src/newgrp.c: sg(1): Refactor, to remove 'else' branch
 3:  56166b6697a6 =  3:  fefe02ecb687 src/newgrp.c: sg(1): -c: Fail if the option is used without an argument
 4:  2b8316a3f78d =  4:  3635176e2822 src/newgrp.c: Split conditional block
 5:  6843c19b360d =  5:  3aa5a64e3a31 src/newgrp.c: Use argv instead of argc to check an argument
 6:  028e5c748b33 =  6:  4bf4e30960e3 src/newgrp.c: Check '-l' and '-' in the documented order
 7:  3a0892de52b6 =  7:  8b29b89fee14 src/newgrp.c: newgrp(1): Remove 'else's after 'goto'
 8:  4246adf9a5f4 =  8:  e007fbcb1eaa src/newgrp.c: De-duplicate check
 9:  1f62687b7ff4 =  9:  36f9a127dfae src/newgrp.c: Separate code handling group name and code handling command
10:  67834ed31be1 = 10:  c74525577028 src/newgrp.c: Refactor conditionals to de-duplicate code
11:  f11890b4f79a = 11:  e23d52c64070 src/newgrp.c: Fix indentation
12:  85706b5ec1be = 12:  4e0209d0d6af src/newgrp.c: Fail if there are trailing (unexpected) arguments
13:  499eadeb203d = 13:  deda72428f86 src/newgrp.c: Remove incorrect comment
14:  3573e24a6f39 = 14:  f9d2fadd0bf4 src/newgrp.c: sg(1): Use 'goto failure;' to fail
15:  d78ede7e76cd = 15:  b91bed51b80c lib/prototypes.h: Fix indentation
16:  89c89007c03c = 16:  7ceadd0af331 src/newgrp.c: Report SHADOW_AUDIT_FAILURE on failure
17:  657823c6f1ca = 17:  d34e55368aac src/newgrp.c: Fix error handling
18:  919b2c77d4ba = 18:  b526d9097b61 src/newgrp.c: syslog_sg(): Use Prog instead of its pattern
19:  28aae35684c0 = 19:  29a64c5b7339 src/newgrp.c: Use idiomatic pointer arithmetic notation
20:  e4266db158a6 = 20:  22ad719bfcbd src/newgrp.c: Remove redundant comment
21:  29c24fed60a0 = 21:  d08282fce523 src/newgrp.c: Accept both -l and - in the same command
22:  df3bb4cbea44 = 22:  9faab479f973 src/newgrp.c: Remove redundant input validation

@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: Document '-l'
5bc5e61 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: sg(1): Refactor, to remove 'else' branch
bf9e61c 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: sg(1): -c: Fail if the option is used without an argument
fefe02e 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Split conditional block
3635176 
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 <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Use argv instead of argc to check an argument
3aa5a64 
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 <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Check '-l' and '-' in the documented order
4bf4e30 
This makes the code more readable, by more closely matching its
documentation.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: newgrp(1): Remove 'else's after 'goto'
8b29b89 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: De-duplicate check
e007fbc 
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 <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Separate code handling group name and code handling com…
36f9a12 
…mand

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

Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Refactor conditionals to de-duplicate code
c745255 
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 <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Fix indentation
e23d52c 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Fail if there are trailing (unexpected) arguments
4e0209d 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Remove incorrect comment
deda724 
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 <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: sg(1): Use 'goto failure;' to fail
f9d2fad 
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 <alx@kernel.org>
@alejandro-colomar
lib/prototypes.h: Fix indentation
b91bed5 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Report SHADOW_AUDIT_FAILURE on failure
7ceadd0 
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 <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Fix error handling
d34e553 
Use 'goto failure;', as we forgot to closelog(3).

Also, as always, remove the redundant nearby comment.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: syslog_sg(): Use Prog instead of its pattern
b526d90 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Use idiomatic pointer arithmetic notation
29a64c5 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Remove redundant comment
22ad719 
The usage() function already documents the syntax.  Let's keep a single
source of truth.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Accept both -l and - in the same command
d08282f 
This is for consistency with su(1).

Reported-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
src/newgrp.c: Remove redundant input validation
9faab47 
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 <tobias@stoeckmann.org>
Suggested-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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