rpms/shadow-utils
5
0
Fork 0
Code Issues Pull Requests Releases Activity
b397976c6c
shadow-utils/shadow-4.15.0-sast-fixes.patch

1414 lines
37 KiB
Diff
Raw Normal View History

Fix static analyzer detected issues Resolves: RHEL-35383 Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
2024-06-18 09:56:02 +00:00
From 4c16416ebc5f0958d58a1ea1e7890eafd9f8bb75 Mon Sep 17 00:00:00 2001
From: Iker Pedrosa <ipedrosa@redhat.com>
Date: Wed, 15 May 2024 12:25:51 +0200
Subject: [PATCH 01/16] port: fix OVERRUN (CWE-119)
```
shadow-4.15.0/lib/port.c:154:2: alias: Assigning: "port.pt_names" = "ttys". "port.pt_names" now points to element 0 of "ttys" (which consists of 65 8-byte elements).
shadow-4.15.0/lib/port.c:155:2: cond_const: Checking "j < 64" implies that "j" is 64 on the false branch.
shadow-4.15.0/lib/port.c:175:2: overrun-local: Overrunning array of 65 8-byte elements at element index 65 (byte offset 527) by dereferencing pointer "port.pt_names + (j + 1)".
173| *cp = '\0';
174| cp++;
175|-> port.pt_names[j + 1] = NULL;
176|
177| /*
```
Resolves: https://issues.redhat.com/browse/RHEL-35383
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
---
lib/port.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/port.c b/lib/port.c
index 05b95651..60ff8989 100644
--- a/lib/port.c
+++ b/lib/port.c
@@ -168,7 +168,7 @@ again:
}
*cp = '\0';
cp++;
- port.pt_names[j + 1] = NULL;
+ port.pt_names[j] = NULL;
/*
* Get the list of user names. It is the second colon
--
2.45.1
From f8fc6371f69930bbd5801284256e182ba35ced2a Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 14:05:31 +0200
Subject: [PATCH 02/16] src/useradd.c: set_defaults(): Fix order of clean-ups
Resources should be freed in the inverse order of the allocation.
This refactor prepares for the following commits, which fix some leaks.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/useradd.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/useradd.c b/src/useradd.c
index 88d8ab7f..56a74559 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -745,10 +745,9 @@ static int set_defaults (void)
def_create_mail_spool, def_log_init));
ret = 0;
setdef_err:
- free(new_file);
- if (prefix[0]) {
+ if (prefix[0])
free(default_file);
- }
+ free(new_file);
return ret;
}
--
2.45.1
From 37ae8827a0869ee4a723954c3c9e7c48165d9b50 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 14:28:50 +0200
Subject: [PATCH 03/16] src/useradd.c: set_defaults(): Rename goto label
This will help add other labels in the following commits.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/useradd.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/src/useradd.c b/src/useradd.c
index 56a74559..bc72e6bc 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -558,7 +558,7 @@ static int set_defaults (void)
fprintf(stderr,
_("%s: cannot create new defaults file: %s\n"),
Prog, strerror(errno));
- goto setdef_err;
+ goto err_free_def;
}
}
@@ -567,7 +567,7 @@ static int set_defaults (void)
fprintf (stderr,
_("%s: cannot create directory for defaults file\n"),
Prog);
- goto setdef_err;
+ goto err_free_def;
}
ret = mkdir(dirname(new_file_dup), 0755);
@@ -576,7 +576,7 @@ static int set_defaults (void)
_("%s: cannot create directory for defaults file\n"),
Prog);
free(new_file_dup);
- goto setdef_err;
+ goto err_free_def;
}
free(new_file_dup);
@@ -588,7 +588,7 @@ static int set_defaults (void)
fprintf (stderr,
_("%s: cannot create new defaults file\n"),
Prog);
- goto setdef_err;
+ goto err_free_def;
}
ofp = fdopen (ofd, "w");
@@ -596,7 +596,7 @@ static int set_defaults (void)
fprintf (stderr,
_("%s: cannot open new defaults file\n"),
Prog);
- goto setdef_err;
+ goto err_free_def;
}
/*
@@ -623,7 +623,7 @@ static int set_defaults (void)
_("%s: line too long in %s: %s..."),
Prog, default_file, buf);
(void) fclose (ifp);
- goto setdef_err;
+ goto err_free_def;
}
}
@@ -702,9 +702,10 @@ static int set_defaults (void)
(void) fflush (ofp);
if ( (ferror (ofp) != 0)
|| (fsync (fileno (ofp)) != 0)
- || (fclose (ofp) != 0)) {
+ || (fclose (ofp) != 0))
+ {
unlink (new_file);
- goto setdef_err;
+ goto err_free_def;
}
/*
@@ -718,7 +719,7 @@ static int set_defaults (void)
_("%s: Cannot create backup file (%s): %s\n"),
Prog, buf, strerror (err));
unlink (new_file);
- goto setdef_err;
+ goto err_free_def;
}
/*
@@ -729,7 +730,7 @@ static int set_defaults (void)
fprintf (stderr,
_("%s: rename: %s: %s\n"),
Prog, new_file, strerror (err));
- goto setdef_err;
+ goto err_free_def;
}
#ifdef WITH_AUDIT
audit_logger (AUDIT_USYS_CONFIG, Prog,
@@ -744,7 +745,8 @@ static int set_defaults (void)
def_inactive, def_expire, def_template,
def_create_mail_spool, def_log_init));
ret = 0;
- setdef_err:
+
+err_free_def:
if (prefix[0])
free(default_file);
free(new_file);
--
2.45.1
From 701fe4cf1aeac9e66fa949369c91d135dbf375d2 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 13:10:46 +0200
Subject: [PATCH 04/16] src/useradd.c: set_defaults(): Do not free(3) the
result of asprintf(3) if it failed
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
See asprintf(3):
RETURN VALUE
When successful, these functions return the number of bytes
printed, just like sprintf(3). If memory allocation wasn’t possi‐
ble, or some other error occurs, these functions will return -1,
and the contents of strp are undefined.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/useradd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/useradd.c b/src/useradd.c
index bc72e6bc..6a3edfe3 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -558,7 +558,7 @@ static int set_defaults (void)
fprintf(stderr,
_("%s: cannot create new defaults file: %s\n"),
Prog, strerror(errno));
- goto err_free_def;
+ goto err_free_new;
}
}
@@ -749,6 +749,7 @@ static int set_defaults (void)
err_free_def:
if (prefix[0])
free(default_file);
+err_free_new:
free(new_file);
return ret;
--
2.45.1
From a74c4b6ae124a55cd272e574e0d056102f331e17 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 13:14:31 +0200
Subject: [PATCH 05/16] src/useradd.c: De-duplicate code
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/useradd.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/useradd.c b/src/useradd.c
index 6a3edfe3..ad2676c1 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -571,14 +571,13 @@ static int set_defaults (void)
}
ret = mkdir(dirname(new_file_dup), 0755);
+ free(new_file_dup);
if (-1 == ret && EEXIST != errno) {
fprintf (stderr,
_("%s: cannot create directory for defaults file\n"),
Prog);
- free(new_file_dup);
goto err_free_def;
}
- free(new_file_dup);
/*
* Create a temporary file to copy the new output to.
--
2.45.1
From e7d1508e076bbf4053faacc0370c6fe43d9c8f04 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 13:40:58 +0200
Subject: [PATCH 06/16] src/useradd.c: Add fmkstemp() to fix file-descriptor
leak
This function creates a temporary file, and returns a FILE pointer to
it. This avoids dealing with both a file descriptor and a FILE pointer,
and correctly deallocating the resources on error.
The code before this patch was leaking the file descriptor if fdopen(3)
failed.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/useradd.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/src/useradd.c b/src/useradd.c
index ad2676c1..e0238457 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -238,6 +238,9 @@ static void create_home (void);
static void create_mail (void);
static void check_uid_range(int rflg, uid_t user_id);
+static FILE *fmkstemp(char *template);
+
+
/*
* fail_exit - undo as much as possible
*/
@@ -524,7 +527,6 @@ static void show_defaults (void)
*/
static int set_defaults (void)
{
- int ofd;
int ret = -1;
bool out_group = false;
bool out_groups = false;
@@ -582,15 +584,7 @@ static int set_defaults (void)
/*
* Create a temporary file to copy the new output to.
*/
- ofd = mkstemp (new_file);
- if (-1 == ofd) {
- fprintf (stderr,
- _("%s: cannot create new defaults file\n"),
- Prog);
- goto err_free_def;
- }
-
- ofp = fdopen (ofd, "w");
+ ofp = fmkstemp(new_file);
if (NULL == ofp) {
fprintf (stderr,
_("%s: cannot open new defaults file\n"),
@@ -2752,3 +2746,23 @@ int main (int argc, char **argv)
return E_SUCCESS;
}
+
+static FILE *
+fmkstemp(char *template)
+{
+ int fd;
+ FILE *fp;
+
+ fd = mkstemp(template);
+ if (fd == -1)
+ return NULL;
+
+ fp = fdopen(fd, "w");
+ if (fp == NULL) {
+ close(fd);
+ unlink(template);
+ return NULL;
+ }
+
+ return fp;
+}
--
2.45.1
From 1ee066ae1e5b39ac42120ad0f6f8af0f102db952 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 13:52:07 +0200
Subject: [PATCH 07/16] src/useradd.c: set_defaults(): Fix FILE* leak
Report:
> shadow-4.15.0/src/useradd.c:575:2: alloc_fn: Storage is returned from allocation function "fdopen".
> shadow-4.15.0/src/useradd.c:575:2: var_assign: Assigning: "ofp" = storage returned from "fdopen(ofd, "w")".
> shadow-4.15.0/src/useradd.c:734:2: leaked_storage: Variable "ofp" going out of scope leaks the storage it points to.
> 732| }
> 733|
> 734|-> return ret;
> 735| }
> 736|
Link: <https://issues.redhat.com/browse/RHEL-35383>
Reported-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/useradd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/useradd.c b/src/useradd.c
index e0238457..347334a6 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -615,7 +615,8 @@ static int set_defaults (void)
fprintf (stderr,
_("%s: line too long in %s: %s..."),
Prog, default_file, buf);
- (void) fclose (ifp);
+ fclose(ifp);
+ fclose(ofp);
goto err_free_def;
}
}
--
2.45.1
From 151f14ad69de8100d25c1974947d53ae40d1448a Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Thu, 16 May 2024 13:52:15 +0200
Subject: [PATCH 08/16] src/usermod.c: Reduce scope of local variables
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/usermod.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/usermod.c b/src/usermod.c
index 0fcf0325..57b58f5b 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -687,11 +687,8 @@ fail_exit (int code)
static void update_group (void)
{
- bool is_member;
- bool was_member;
- bool changed;
- const struct group *grp;
- struct group *ngrp;
+ bool changed;
+ const struct group *grp;
changed = false;
@@ -700,6 +697,9 @@ static void update_group (void)
* the user is a member of.
*/
while ((grp = gr_next ()) != NULL) {
+ bool is_member;
+ bool was_member;
+ struct group *ngrp;
/*
* See if the user specified this group as one of their
* concurrent groups.
@@ -799,12 +799,8 @@ static void update_group (void)
#ifdef SHADOWGRP
static void update_gshadow (void)
{
- bool is_member;
- bool was_member;
- bool was_admin;
- bool changed;
- const struct sgrp *sgrp;
- struct sgrp *nsgrp;
+ bool changed;
+ const struct sgrp *sgrp;
changed = false;
@@ -813,6 +809,10 @@ static void update_gshadow (void)
* that the user is a member of.
*/
while ((sgrp = sgr_next ()) != NULL) {
+ bool is_member;
+ bool was_member;
+ bool was_admin;
+ struct sgrp *nsgrp;
/*
* See if the user was a member of this group
--
2.45.1
From b089a63ab38f69c32d099320fe8181802f7f4092 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Thu, 16 May 2024 13:49:34 +0200
Subject: [PATCH 09/16] src/usermod.c: Rename update_group() =>
update_group_file()
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/usermod.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/usermod.c b/src/usermod.c
index 57b58f5b..aaa83d7d 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -178,7 +178,7 @@ NORETURN static void usage (int status);
static void new_pwent (struct passwd *);
static void new_spent (struct spwd *);
NORETURN static void fail_exit (int);
-static void update_group (void);
+static void update_group_file(void);
#ifdef SHADOWGRP
static void update_gshadow (void);
@@ -685,7 +685,8 @@ fail_exit (int code)
}
-static void update_group (void)
+static void
+update_group_file(void)
{
bool changed;
const struct group *grp;
@@ -950,7 +951,7 @@ static void update_gshadow (void)
*/
static void grp_update (void)
{
- update_group ();
+ update_group_file();
#ifdef SHADOWGRP
if (is_shadow_grp) {
update_gshadow ();
--
2.45.1
From 81bc78ec5cdd59790bc7c591c9d1f66bd4d7b78e Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 02:11:22 +0200
Subject: [PATCH 10/16] src/usermod.c: Rename update_gshadow() =>
update_gshadow_file()
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/usermod.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/usermod.c b/src/usermod.c
index aaa83d7d..3048f801 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -181,7 +181,7 @@ NORETURN static void fail_exit (int);
static void update_group_file(void);
#ifdef SHADOWGRP
-static void update_gshadow (void);
+static void update_gshadow_file(void);
#endif
static void grp_update (void);
@@ -798,7 +798,8 @@ update_group_file(void)
}
#ifdef SHADOWGRP
-static void update_gshadow (void)
+static void
+update_gshadow_file(void)
{
bool changed;
const struct sgrp *sgrp;
@@ -954,7 +955,7 @@ static void grp_update (void)
update_group_file();
#ifdef SHADOWGRP
if (is_shadow_grp) {
- update_gshadow ();
+ update_gshadow_file();
}
#endif
}
--
2.45.1
From 61964aa06b9e6e0643a6519f64290f18ac04867f Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Thu, 16 May 2024 13:54:06 +0200
Subject: [PATCH 11/16] src/usermod.c: update_group_file(): Fix RESOURCE_LEAK
(CWE-772)
Report:
> shadow-4.15.0/src/usermod.c:734:3: alloc_fn: Storage is returned from allocation function "__gr_dup".
> shadow-4.15.0/src/usermod.c:734:3: var_assign: Assigning: "ngrp" = storage returned from "__gr_dup(grp)".
> shadow-4.15.0/src/usermod.c:815:1: leaked_storage: Variable "ngrp" going out of scope leaks the storage it points to.
> 813| gr_free(ngrp);
> 814| }
> 815|-> }
> 816|
> 817| #ifdef SHADOWGRP
Link: https://issues.redhat.com/browse/RHEL-35383
Reported-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/usermod.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/usermod.c b/src/usermod.c
index 3048f801..e0cfdd83 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -780,9 +780,8 @@ update_group_file(void)
SYSLOG ((LOG_INFO, "add '%s' to group '%s'",
user_newname, ngrp->gr_name));
}
- if (!changed) {
- continue;
- }
+ if (!changed)
+ goto free_ngrp;
changed = false;
if (gr_update (ngrp) == 0) {
@@ -793,6 +792,7 @@ update_group_file(void)
fail_exit (E_GRP_UPDATE);
}
+free_ngrp:
gr_free(ngrp);
}
}
--
2.45.1
From 71a3238b7996285fc3c8dec841244ba95d663fa5 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 02:15:15 +0200
Subject: [PATCH 12/16] src/usermod.c: update_gshadow_file(): Fix RESOURCE_LEAK
(CWE-772)
Report:
> shadow-4.15.0/src/usermod.c:864:3: alloc_fn: Storage is returned from allocation function "__sgr_dup".
> shadow-4.15.0/src/usermod.c:864:3: var_assign: Assigning: "nsgrp" = storage returned from "__sgr_dup(sgrp)".
> shadow-4.15.0/src/usermod.c:964:1: leaked_storage: Variable "nsgrp" going out of scope leaks the storage it points to.
> 962| free (nsgrp);
> 963| }
> 964|-> }
> 965| #endif /* SHADOWGRP */
> 966|
Link: https://issues.redhat.com/browse/RHEL-35383
Reported-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/usermod.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/usermod.c b/src/usermod.c
index e0cfdd83..bb5d3535 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -921,9 +921,8 @@ update_gshadow_file(void)
SYSLOG ((LOG_INFO, "add '%s' to shadow group '%s'",
user_newname, nsgrp->sg_name));
}
- if (!changed) {
- continue;
- }
+ if (!changed)
+ goto free_nsgrp;
changed = false;
@@ -939,6 +938,7 @@ update_gshadow_file(void)
fail_exit (E_GRP_UPDATE);
}
+free_nsgrp:
free (nsgrp);
}
}
--
2.45.1
From 68d42a8fbe42b89cf13d3f672ad8502dbaf05835 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Thu, 16 May 2024 14:02:54 +0200
Subject: [PATCH 13/16] src/usermod.c: update_group_file(): Reduce scope of
local variable
After _every_ iteration, 'changed' is always 'false'. We don't need to
have it outside of the loop.
See:
$ grepc update_group_file . \
| grep -e changed -e goto -e continue -e break -e free_ngrp -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
bool changed;
changed = false;
while ((grp = gr_next ()) != NULL) {
if (!was_member && !is_member) {
continue;
}
if (was_member) {
if ((!Gflg) || is_member) {
if (lflg) {
changed = true;
}
} else {
changed = true;
}
} else if (is_member) {
changed = true;
}
if (!changed)
goto free_ngrp;
changed = false;
free_ngrp:
}
}
This was already true in the commit that introduced the code:
$ git show 45c6603cc:src/usermod.c \
| grepc update_group \
| grep -e changed -e goto -e break -e continue -e '\<if\>' -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
int changed;
changed = 0;
while ((grp = gr_next())) {
* See if the user specified this group as one of their
if (!was_member && !is_member)
continue;
if (was_member && (!Gflg || is_member)) {
if (lflg) {
changed = 1;
}
} else if (was_member && Gflg && !is_member) {
changed = 1;
} else if (!was_member && Gflg && is_member) {
changed = 1;
}
if (!changed)
continue;
changed = 0;
}
}
Fixes: 45c6603cc86c ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/usermod.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/usermod.c b/src/usermod.c
index bb5d3535..30f47b8a 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -688,19 +688,20 @@ fail_exit (int code)
static void
update_group_file(void)
{
- bool changed;
const struct group *grp;
- changed = false;
-
/*
* Scan through the entire group file looking for the groups that
* the user is a member of.
*/
while ((grp = gr_next ()) != NULL) {
+ bool changed;
bool is_member;
bool was_member;
struct group *ngrp;
+
+ changed = false;
+
/*
* See if the user specified this group as one of their
* concurrent groups.
@@ -783,7 +784,6 @@ update_group_file(void)
if (!changed)
goto free_ngrp;
- changed = false;
if (gr_update (ngrp) == 0) {
fprintf (stderr,
_("%s: failed to prepare the new %s entry '%s'\n"),
--
2.45.1
From da77a82ecbc90e89808f143e7fa2abb7650f50d7 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 02:19:46 +0200
Subject: [PATCH 14/16] src/usermod.c: update_gshadow_file(): Reduce scope of
local variable
After _every_ iteration, 'changed' is always 'false'. We don't need to
have it outside of the loop.
See:
$ grepc update_gshadow_file . \
| grep -e changed -e goto -e continue -e break -e free_ngrp -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
bool changed;
changed = false;
while ((sgrp = sgr_next ()) != NULL) {
if (!was_member && !was_admin && !is_member) {
continue;
}
if (was_admin && lflg) {
changed = true;
}
if (was_member) {
if ((!Gflg) || is_member) {
if (lflg) {
changed = true;
}
} else {
changed = true;
}
} else if (is_member) {
changed = true;
}
if (!changed)
goto free_nsgrp;
changed = false;
}
}
This was already true in the commit that introduced the code:
$ git show 45c6603cc:src/usermod.c \
| grepc update_gshadow \
| grep -e changed -e goto -e break -e continue -e '\<if\>' -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
int changed;
changed = 0;
while ((sgrp = sgr_next())) {
* See if the user was a member of this group
* See if the user was an administrator of this group
* See if the user specified this group as one of their
if (!was_member && !was_admin && !is_member)
continue;
if (was_admin && lflg) {
changed = 1;
}
if (was_member && (!Gflg || is_member)) {
if (lflg) {
changed = 1;
}
} else if (was_member && Gflg && !is_member) {
changed = 1;
} else if (!was_member && Gflg && is_member) {
changed = 1;
}
if (!changed)
continue;
changed = 0;
}
}
Fixes: 45c6603cc86c ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/usermod.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/usermod.c b/src/usermod.c
index 30f47b8a..7b1e0581 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -801,21 +801,21 @@ free_ngrp:
static void
update_gshadow_file(void)
{
- bool changed;
const struct sgrp *sgrp;
- changed = false;
-
/*
* Scan through the entire shadow group file looking for the groups
* that the user is a member of.
*/
while ((sgrp = sgr_next ()) != NULL) {
+ bool changed;
bool is_member;
bool was_member;
bool was_admin;
struct sgrp *nsgrp;
+ changed = false;
+
/*
* See if the user was a member of this group
*/
@@ -924,8 +924,6 @@ update_gshadow_file(void)
if (!changed)
goto free_nsgrp;
- changed = false;
-
/*
* Update the group entry to reflect the changes.
*/
--
2.45.1
From adf37cccd0fa4ce7d05644514b0af57fe71905c3 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Thu, 16 May 2024 14:12:09 +0200
Subject: [PATCH 15/16] src/usermod.c: update_group(): Add helper function
Keep the while loop in the outer function, and move the iteration code
to this new helper. This makes it a bit more readable.
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/usermod.c | 167 ++++++++++++++++++++++++++------------------------
1 file changed, 87 insertions(+), 80 deletions(-)
diff --git a/src/usermod.c b/src/usermod.c
index 7b1e0581..4ea11376 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -179,6 +179,7 @@ static void new_pwent (struct passwd *);
static void new_spent (struct spwd *);
NORETURN static void fail_exit (int);
static void update_group_file(void);
+static void update_group(const struct group *grp);
#ifdef SHADOWGRP
static void update_gshadow_file(void);
@@ -694,109 +695,115 @@ update_group_file(void)
* Scan through the entire group file looking for the groups that
* the user is a member of.
*/
- while ((grp = gr_next ()) != NULL) {
- bool changed;
- bool is_member;
- bool was_member;
- struct group *ngrp;
+ while ((grp = gr_next()) != NULL)
+ update_group(grp);
+}
- changed = false;
- /*
- * See if the user specified this group as one of their
- * concurrent groups.
- */
- was_member = is_on_list (grp->gr_mem, user_name);
- is_member = Gflg && ( (was_member && aflg)
- || is_on_list (user_groups, grp->gr_name));
+static void
+update_group(const struct group *grp)
+{
+ bool changed;
+ bool is_member;
+ bool was_member;
+ struct group *ngrp;
- if (!was_member && !is_member) {
- continue;
- }
+ changed = false;
- /*
- * If rflg+Gflg is passed in AKA -rG invert is_member flag, which removes
- * mentioned groups while leaving the others.
- */
- if (Gflg && rflg) {
- is_member = !is_member;
- }
+ /*
+ * See if the user specified this group as one of their
+ * concurrent groups.
+ */
+ was_member = is_on_list (grp->gr_mem, user_name);
+ is_member = Gflg && ( (was_member && aflg)
+ || is_on_list (user_groups, grp->gr_name));
- ngrp = __gr_dup (grp);
- if (NULL == ngrp) {
- fprintf (stderr,
- _("%s: Out of memory. Cannot update %s.\n"),
- Prog, gr_dbname ());
- fail_exit (E_GRP_UPDATE);
- }
+ if (!was_member && !is_member)
+ return;
- if (was_member) {
- if ((!Gflg) || is_member) {
- /* User was a member and is still a member
- * of this group.
- * But the user might have been renamed.
- */
- if (lflg) {
- ngrp->gr_mem = del_list (ngrp->gr_mem,
- user_name);
- ngrp->gr_mem = add_list (ngrp->gr_mem,
- user_newname);
- changed = true;
-#ifdef WITH_AUDIT
- audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
- "changing group member",
- user_newname, AUDIT_NO_ID, 1);
-#endif
- SYSLOG ((LOG_INFO,
- "change '%s' to '%s' in group '%s'",
- user_name, user_newname,
- ngrp->gr_name));
- }
- } else {
- /* User was a member but is no more a
- * member of this group.
- */
- ngrp->gr_mem = del_list (ngrp->gr_mem, user_name);
+ /*
+ * If rflg+Gflg is passed in AKA -rG invert is_member flag, which removes
+ * mentioned groups while leaving the others.
+ */
+ if (Gflg && rflg) {
+ is_member = !is_member;
+ }
+
+ ngrp = __gr_dup (grp);
+ if (NULL == ngrp) {
+ fprintf (stderr,
+ _("%s: Out of memory. Cannot update %s.\n"),
+ Prog, gr_dbname ());
+ fail_exit (E_GRP_UPDATE);
+ }
+
+ if (was_member) {
+ if ((!Gflg) || is_member) {
+ /* User was a member and is still a member
+ * of this group.
+ * But the user might have been renamed.
+ */
+ if (lflg) {
+ ngrp->gr_mem = del_list (ngrp->gr_mem,
+ user_name);
+ ngrp->gr_mem = add_list (ngrp->gr_mem,
+ user_newname);
changed = true;
#ifdef WITH_AUDIT
audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
- "removing group member",
- user_name, AUDIT_NO_ID, 1);
+ "changing group member",
+ user_newname, AUDIT_NO_ID, 1);
#endif
SYSLOG ((LOG_INFO,
- "delete '%s' from group '%s'",
- user_name, ngrp->gr_name));
+ "change '%s' to '%s' in group '%s'",
+ user_name, user_newname,
+ ngrp->gr_name));
}
- } else if (is_member) {
- /* User was not a member but is now a member this
- * group.
+ } else {
+ /* User was a member but is no more a
+ * member of this group.
*/
- ngrp->gr_mem = add_list (ngrp->gr_mem, user_newname);
+ ngrp->gr_mem = del_list (ngrp->gr_mem, user_name);
changed = true;
#ifdef WITH_AUDIT
audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
- "adding user to group",
- user_name, AUDIT_NO_ID, 1);
+ "removing group member",
+ user_name, AUDIT_NO_ID, 1);
#endif
- SYSLOG ((LOG_INFO, "add '%s' to group '%s'",
- user_newname, ngrp->gr_name));
+ SYSLOG ((LOG_INFO,
+ "delete '%s' from group '%s'",
+ user_name, ngrp->gr_name));
}
- if (!changed)
- goto free_ngrp;
+ } else if (is_member) {
+ /* User was not a member but is now a member this
+ * group.
+ */
+ ngrp->gr_mem = add_list (ngrp->gr_mem, user_newname);
+ changed = true;
+#ifdef WITH_AUDIT
+ audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
+ "adding user to group",
+ user_name, AUDIT_NO_ID, 1);
+#endif
+ SYSLOG ((LOG_INFO, "add '%s' to group '%s'",
+ user_newname, ngrp->gr_name));
+ }
+ if (!changed)
+ goto free_ngrp;
- if (gr_update (ngrp) == 0) {
- fprintf (stderr,
- _("%s: failed to prepare the new %s entry '%s'\n"),
- Prog, gr_dbname (), ngrp->gr_name);
- SYSLOG ((LOG_WARN, "failed to prepare the new %s entry '%s'", gr_dbname (), ngrp->gr_name));
- fail_exit (E_GRP_UPDATE);
- }
+ if (gr_update (ngrp) == 0) {
+ fprintf (stderr,
+ _("%s: failed to prepare the new %s entry '%s'\n"),
+ Prog, gr_dbname (), ngrp->gr_name);
+ SYSLOG ((LOG_WARN, "failed to prepare the new %s entry '%s'", gr_dbname (), ngrp->gr_name));
+ fail_exit (E_GRP_UPDATE);
+ }
free_ngrp:
- gr_free(ngrp);
- }
+ gr_free(ngrp);
}
+
#ifdef SHADOWGRP
static void
update_gshadow_file(void)
--
2.45.1
From d8e6a8b99b4d844328d875287babf6e13860d464 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 02:29:46 +0200
Subject: [PATCH 16/16] src/usermod.c: update_gshadow(): Add helper function
Keep the while loop in the outer function, and move the iteration code
to this new helper. This makes it a bit more readable.
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
src/usermod.c | 223 ++++++++++++++++++++++++++------------------------
1 file changed, 116 insertions(+), 107 deletions(-)
diff --git a/src/usermod.c b/src/usermod.c
index 4ea11376..f8896984 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -183,6 +183,7 @@ static void update_group(const struct group *grp);
#ifdef SHADOWGRP
static void update_gshadow_file(void);
+static void update_gshadow(const struct sgrp *sgrp);
#endif
static void grp_update (void);
@@ -814,141 +815,149 @@ update_gshadow_file(void)
* Scan through the entire shadow group file looking for the groups
* that the user is a member of.
*/
- while ((sgrp = sgr_next ()) != NULL) {
- bool changed;
- bool is_member;
- bool was_member;
- bool was_admin;
- struct sgrp *nsgrp;
+ while ((sgrp = sgr_next()) != NULL)
+ update_gshadow(sgrp);
+}
+#endif /* SHADOWGRP */
- changed = false;
- /*
- * See if the user was a member of this group
- */
- was_member = is_on_list (sgrp->sg_mem, user_name);
+#ifdef SHADOWGRP
+static void
+update_gshadow(const struct sgrp *sgrp)
+{
+ bool changed;
+ bool is_member;
+ bool was_member;
+ bool was_admin;
+ struct sgrp *nsgrp;
- /*
- * See if the user was an administrator of this group
- */
- was_admin = is_on_list (sgrp->sg_adm, user_name);
+ changed = false;
- /*
- * See if the user specified this group as one of their
- * concurrent groups.
- */
- is_member = Gflg && ( (was_member && aflg)
- || is_on_list (user_groups, sgrp->sg_name));
+ /*
+ * See if the user was a member of this group
+ */
+ was_member = is_on_list (sgrp->sg_mem, user_name);
- if (!was_member && !was_admin && !is_member) {
- continue;
- }
+ /*
+ * See if the user was an administrator of this group
+ */
+ was_admin = is_on_list (sgrp->sg_adm, user_name);
- /*
- * If rflg+Gflg is passed in AKA -rG invert is_member, to remove targeted
- * groups while leaving the user apart of groups not mentioned
- */
- if (Gflg && rflg) {
- is_member = !is_member;
- }
+ /*
+ * See if the user specified this group as one of their
+ * concurrent groups.
+ */
+ is_member = Gflg && ( (was_member && aflg)
+ || is_on_list (user_groups, sgrp->sg_name));
- nsgrp = __sgr_dup (sgrp);
- if (NULL == nsgrp) {
- fprintf (stderr,
- _("%s: Out of memory. Cannot update %s.\n"),
- Prog, sgr_dbname ());
- fail_exit (E_GRP_UPDATE);
- }
+ if (!was_member && !was_admin && !is_member)
+ return;
- if (was_admin && lflg) {
- /* User was an admin of this group but the user
- * has been renamed.
- */
- nsgrp->sg_adm = del_list (nsgrp->sg_adm, user_name);
- nsgrp->sg_adm = add_list (nsgrp->sg_adm, user_newname);
- changed = true;
-#ifdef WITH_AUDIT
- audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
- "changing admin name in shadow group",
- user_name, AUDIT_NO_ID, 1);
-#endif
- SYSLOG ((LOG_INFO,
- "change admin '%s' to '%s' in shadow group '%s'",
- user_name, user_newname, nsgrp->sg_name));
- }
-
- if (was_member) {
- if ((!Gflg) || is_member) {
- /* User was a member and is still a member
- * of this group.
- * But the user might have been renamed.
- */
- if (lflg) {
- nsgrp->sg_mem = del_list (nsgrp->sg_mem,
- user_name);
- nsgrp->sg_mem = add_list (nsgrp->sg_mem,
- user_newname);
- changed = true;
+ /*
+ * If rflg+Gflg is passed in AKA -rG invert is_member, to remove targeted
+ * groups while leaving the user apart of groups not mentioned
+ */
+ if (Gflg && rflg) {
+ is_member = !is_member;
+ }
+
+ nsgrp = __sgr_dup (sgrp);
+ if (NULL == nsgrp) {
+ fprintf (stderr,
+ _("%s: Out of memory. Cannot update %s.\n"),
+ Prog, sgr_dbname ());
+ fail_exit (E_GRP_UPDATE);
+ }
+
+ if (was_admin && lflg) {
+ /* User was an admin of this group but the user
+ * has been renamed.
+ */
+ nsgrp->sg_adm = del_list (nsgrp->sg_adm, user_name);
+ nsgrp->sg_adm = add_list (nsgrp->sg_adm, user_newname);
+ changed = true;
#ifdef WITH_AUDIT
- audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
- "changing member in shadow group",
- user_name, AUDIT_NO_ID, 1);
+ audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
+ "changing admin name in shadow group",
+ user_name, AUDIT_NO_ID, 1);
#endif
- SYSLOG ((LOG_INFO,
- "change '%s' to '%s' in shadow group '%s'",
- user_name, user_newname,
- nsgrp->sg_name));
- }
- } else {
- /* User was a member but is no more a
- * member of this group.
- */
- nsgrp->sg_mem = del_list (nsgrp->sg_mem, user_name);
+ SYSLOG ((LOG_INFO,
+ "change admin '%s' to '%s' in shadow group '%s'",
+ user_name, user_newname, nsgrp->sg_name));
+ }
+
+ if (was_member) {
+ if ((!Gflg) || is_member) {
+ /* User was a member and is still a member
+ * of this group.
+ * But the user might have been renamed.
+ */
+ if (lflg) {
+ nsgrp->sg_mem = del_list (nsgrp->sg_mem,
+ user_name);
+ nsgrp->sg_mem = add_list (nsgrp->sg_mem,
+ user_newname);
changed = true;
#ifdef WITH_AUDIT
audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
- "removing user from shadow group",
- user_name, AUDIT_NO_ID, 1);
+ "changing member in shadow group",
+ user_name, AUDIT_NO_ID, 1);
#endif
SYSLOG ((LOG_INFO,
- "delete '%s' from shadow group '%s'",
- user_name, nsgrp->sg_name));
+ "change '%s' to '%s' in shadow group '%s'",
+ user_name, user_newname,
+ nsgrp->sg_name));
}
- } else if (is_member) {
- /* User was not a member but is now a member this
- * group.
+ } else {
+ /* User was a member but is no more a
+ * member of this group.
*/
- nsgrp->sg_mem = add_list (nsgrp->sg_mem, user_newname);
+ nsgrp->sg_mem = del_list (nsgrp->sg_mem, user_name);
changed = true;
#ifdef WITH_AUDIT
audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
- "adding user to shadow group",
- user_newname, AUDIT_NO_ID, 1);
+ "removing user from shadow group",
+ user_name, AUDIT_NO_ID, 1);
#endif
- SYSLOG ((LOG_INFO, "add '%s' to shadow group '%s'",
- user_newname, nsgrp->sg_name));
+ SYSLOG ((LOG_INFO,
+ "delete '%s' from shadow group '%s'",
+ user_name, nsgrp->sg_name));
}
- if (!changed)
- goto free_nsgrp;
-
- /*
- * Update the group entry to reflect the changes.
+ } else if (is_member) {
+ /* User was not a member but is now a member this
+ * group.
*/
- if (sgr_update (nsgrp) == 0) {
- fprintf (stderr,
- _("%s: failed to prepare the new %s entry '%s'\n"),
- Prog, sgr_dbname (), nsgrp->sg_name);
- SYSLOG ((LOG_WARN, "failed to prepare the new %s entry '%s'",
- sgr_dbname (), nsgrp->sg_name));
- fail_exit (E_GRP_UPDATE);
- }
+ nsgrp->sg_mem = add_list (nsgrp->sg_mem, user_newname);
+ changed = true;
+#ifdef WITH_AUDIT
+ audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
+ "adding user to shadow group",
+ user_newname, AUDIT_NO_ID, 1);
+#endif
+ SYSLOG ((LOG_INFO, "add '%s' to shadow group '%s'",
+ user_newname, nsgrp->sg_name));
+ }
+ if (!changed)
+ goto free_nsgrp;
-free_nsgrp:
- free (nsgrp);
+ /*
+ * Update the group entry to reflect the changes.
+ */
+ if (sgr_update (nsgrp) == 0) {
+ fprintf (stderr,
+ _("%s: failed to prepare the new %s entry '%s'\n"),
+ Prog, sgr_dbname (), nsgrp->sg_name);
+ SYSLOG ((LOG_WARN, "failed to prepare the new %s entry '%s'",
+ sgr_dbname (), nsgrp->sg_name));
+ fail_exit (E_GRP_UPDATE);
}
+
+free_nsgrp:
+ free (nsgrp);
}
#endif /* SHADOWGRP */
+
/*
* grp_update - add user to secondary group set
*
--
2.45.1
Reference in New Issue Copy Permalink