From bae11e18f8e99a6c75963a9edb80c7b406ca493b Mon Sep 17 00:00:00 2001 From: Artem Semenov Date: Wed, 29 Jul 2026 15:23:40 +0300 Subject: [PATCH 1/5] fix: src/groupmod.c: borrow gr_mem in new gshadow-entry branch The pflg branch that synthesizes a fresh gshadow entry eagerly duplicated the member list: sgrp.sg_mem = dup_list(grp.gr_mem); That owned copy is only ever released in the user_list != NULL path (where sgrp.sg_mem is unconditionally overwritten by dup_list()/ xmalloc_T() before it is freed). When groupmod is invoked with -p and no member list (user_list == NULL), sgrp.sg_mem keeps the eager duplicate, which nothing frees -> a leak on every such call. Borrow grp.gr_mem instead. sgr_update() deep-copies the entry, so the borrowed array is only read. The borrow stays live exclusively while user_list == NULL (never freed); as soon as a member list is given, sgrp.sg_mem is reassigned to an owned array before the free at the end of grp_update(). No use-after-free, no double free. Signed-off-by: Artem Semenov --- src/groupmod.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/groupmod.c b/src/groupmod.c index f847c9939b..17bf02a299 100644 --- a/src/groupmod.c +++ b/src/groupmod.c @@ -241,7 +241,7 @@ grp_update(void) sgrp.sg_namp = xstrdup (grp.gr_name); sgrp.sg_passwd = xstrdup (grp.gr_passwd); sgrp.sg_adm = ∅ - sgrp.sg_mem = dup_list (grp.gr_mem); + sgrp.sg_mem = grp.gr_mem; new_sgent (&sgrp); osgrp = &sgrp; /* entry needs to be committed */ } From e7c74d74bd9987969ee6d8c3cd313d9c40f0ad2c Mon Sep 17 00:00:00 2001 From: Artem Semenov Date: Wed, 29 Jul 2026 15:23:22 +0300 Subject: [PATCH 2/5] fix: src/groupmod.c: dup_list() unconditionally in append path In the -a (append) path, grp.gr_mem / sgrp.sg_mem were duplicated only when the existing member list was non-empty: if (NULL != grp.gr_mem[0]) grp.gr_mem = dup_list(grp.gr_mem); grp is a shallow copy of the located entry (grp = *ogrp), so grp.gr_mem aliases the member array owned by the in-core database entry ogrp. When the list is empty, dup_list() is skipped and grp.gr_mem keeps pointing into ogrp. This breaks once add_list() owns and frees its argument (see the lib/list.c changes): add_list() is /*@only@*/ and frees the old container before returning the new one. In the empty-list case that container is ogrp->gr_mem, so the database entry is left with a dangling gr_mem. gr_update()/sgr_update() do not free their input, so the fault is not there: commonio_update() deep-copies the argument via cio_dup() (__gr_dup()) and only reads it. Its cio_free() calls hit other memory -- the internal copy on error paths, and cio_free(p->eptr) on the *previous* entry, which is ogrp. That gr_free(ogrp) walks ogrp->gr_mem, the array add_list() already freed: a use-after-free while iterating it and a double free of the container. The bug stays latent by itself (add_list() did not previously free its argument) and only turns into a hard fault together with the lib/list.c fixes, which is likely why it went unnoticed. Dropping the NULL-check guards so dup_list() always runs gives add_list() an owned copy to free and leaves ogrp->gr_mem for commonio_update() to release exactly once. No use-after-free, no double free. Signed-off-by: Artem Semenov Reviewed-by: Alejandro Colomar --- src/groupmod.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/groupmod.c b/src/groupmod.c index 17bf02a299..2210116c0d 100644 --- a/src/groupmod.c +++ b/src/groupmod.c @@ -259,8 +259,7 @@ grp_update(void) grp.gr_mem[0] = NULL; } else { // append to existing groups - if (NULL != grp.gr_mem[0]) - grp.gr_mem = dup_list (grp.gr_mem); + grp.gr_mem = dup_list (grp.gr_mem); } #ifdef SHADOWGRP if (NULL != osgrp) { @@ -268,8 +267,7 @@ grp_update(void) sgrp.sg_mem = xmalloc_T(1, char *); sgrp.sg_mem[0] = NULL; } else { - if (NULL != sgrp.sg_mem[0]) - sgrp.sg_mem = dup_list(sgrp.sg_mem); + sgrp.sg_mem = dup_list(sgrp.sg_mem); } } #endif /* SHADOWGRP */ From 19d3384fdbeffa4e1184b1cdef1aa7bd527ca965 Mon Sep 17 00:00:00 2001 From: Artem Semenov Date: Wed, 29 Jul 2026 12:44:49 +0300 Subject: [PATCH 3/5] fix: lib/list.c: add_list(): free the replaced list add_list() allocates a new array, copies the existing pointers into it and appends the new member, but never frees the old array it was handed. The parameter is annotated /*@only@*/, so add_list() owns it; free the old container before returning the new one. This leaked the previous member array on every addition, e.g. in useradd's grp_update(): valgrind --leak-check=full --show-leak-kinds=all \ src/useradd -M -N -G grp1,...,grp10 alice before this commit: 80 bytes in 10 blocks are definitely lost at 0x4849388: reallocarray by __gr_dup (groupmem.c:52) by grp_update (useradd.c:1042) 80 bytes in 10 blocks are definitely lost at 0x4849388: reallocarray by __sgr_dup (sgroupio.c:86) by grp_update (useradd.c:1104) definitely lost: 160 bytes in 20 blocks after this commit: definitely lost: 0 bytes in 0 blocks Two callers relied on add_list() not freeing its argument and must be given an owned allocation now: - groupadd: new_grent()/new_sgent() pointed the member lists at a shared static sentinel (&empty_list), which free() must never be applied to. Give groupadd owned heap lists (comma_to_list("")) and release them once the entry has been stored (gr_update()/sgr_update() keep a copy). This also drops the member arrays groupadd leaked: valgrind ... src/groupadd -U alice,bob newgrp before this commit: definitely lost: 80 bytes in 4 blocks after this commit: definitely lost: 0 bytes in 0 blocks - groupmod: release owned member lists built in the append path once gr_update()/sgr_update() have stored their copy; this plugs the pre-existing leak of the duplicated array. valgrind ... src/groupmod -a -U alice emptygrp before this commit: definitely lost: 32 bytes in 2 blocks after this commit: definitely lost: 0 bytes in 0 blocks Signed-off-by: Artem Semenov Reviewed-by: Alejandro Colomar --- lib/list.c | 2 ++ src/groupadd.c | 16 +++++++++++----- src/groupmod.c | 10 ++++++++++ 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/lib/list.c b/lib/list.c index 8fa0e085ac..e505e1e3c6 100644 --- a/lib/list.c +++ b/lib/list.c @@ -67,6 +67,8 @@ add_list(/*@returned@*/ /*@only@*/char **list, const char *member) tmp[i] = xstrdup (member); tmp[i+1] = NULL; + free (list); + return tmp; } diff --git a/src/groupadd.c b/src/groupadd.c index 483d282283..6e1dedfe31 100644 --- a/src/groupadd.c +++ b/src/groupadd.c @@ -67,8 +67,6 @@ static const char Prog[] = "groupadd"; static /*@null@*/char *group_name; static gid_t group_id; static /*@null@*/char *group_passwd; -static /*@null@*/char *empty_list = NULL; - static const char *prefix = ""; static char *user_list; @@ -151,7 +149,7 @@ static void new_grent (struct group *grent) grent->gr_passwd = SHADOW_PASSWD_STRING; /* XXX warning: const */ } grent->gr_gid = group_id; - grent->gr_mem = &empty_list; + grent->gr_mem = comma_to_list(""); } #ifdef SHADOWGRP @@ -170,8 +168,8 @@ static void new_sgent (struct sgrp *sgent) } else { sgent->sg_passwd = "!"; /* XXX warning: const */ } - sgent->sg_adm = &empty_list; - sgent->sg_mem = &empty_list; + sgent->sg_adm = comma_to_list(""); + sgent->sg_mem = comma_to_list(""); } #endif /* SHADOWGRP */ @@ -247,6 +245,14 @@ grp_update(void) Prog, sgr_dbname (), sgrp.sg_namp); fail_exit (E_GRP_UPDATE); } +#endif /* SHADOWGRP */ + free_list(grp.gr_mem); + free(grp.gr_mem); +#ifdef SHADOWGRP + free_list(sgrp.sg_mem); + free(sgrp.sg_mem); + free_list(sgrp.sg_adm); + free(sgrp.sg_adm); #endif /* SHADOWGRP */ } diff --git a/src/groupmod.c b/src/groupmod.c index 2210116c0d..cac27214c4 100644 --- a/src/groupmod.c +++ b/src/groupmod.c @@ -325,6 +325,16 @@ grp_update(void) } } #endif /* SHADOWGRP */ + if (NULL != user_list) { + free_list(grp.gr_mem); + free(grp.gr_mem); +#ifdef SHADOWGRP + if (NULL != osgrp) { + free_list(sgrp.sg_mem); + free(sgrp.sg_mem); + } +#endif /* SHADOWGRP */ + } } /* From a7465f3e7a2929f629f0147238b273382f3caf06 Mon Sep 17 00:00:00 2001 From: Artem Semenov Date: Thu, 30 Jul 2026 16:48:05 +0300 Subject: [PATCH 4/5] fix: lib/list.c: del_list(): free the removed entry's string When del_list() removes a member it allocates a new array and copies the surviving pointers into it, but it never freed the removed member's string. Once the new array is returned that string becomes unreachable. Free the removed entry's string as it is skipped in the copy loop. The freeing happens after the new array has been allocated, and the freed string is never compared against 'member' again, so the loop stays safe. Signed-off-by: Artem Semenov Reviewed-by: Alejandro Colomar --- lib/list.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/list.c b/lib/list.c index e505e1e3c6..4600d3a8e3 100644 --- a/lib/list.c +++ b/lib/list.c @@ -121,6 +121,8 @@ del_list(/*@returned@*/ /*@only@*/char **list, const char *member) if (!streq(list[i], member)) { tmp[j] = list[i]; j++; + } else { + free (list[i]); } } From 160e162a8c5388ba5b66d22c12698f4116429d24 Mon Sep 17 00:00:00 2001 From: Artem Semenov Date: Thu, 30 Jul 2026 16:48:17 +0300 Subject: [PATCH 5/5] fix: lib/list.c: del_list(): free the old list container del_list() allocates a new array for the surviving pointers but never freed the old container array, which becomes unreachable once the new array is returned. Free the old container after the survivors have been transferred to the new array. The parameter is /*@only@*/, so del_list() owns it and is responsible for releasing it. valgrind --leak-check=full --show-leak-kinds=all src/userdel -P bob, before this commit: 200 (160 direct, 40 indirect) bytes in 10 blocks are definitely lost at 0x4849388: reallocarray by __gr_dup (groupmem.c:52) by update_groups (userdel.c:197) 200 (160 direct, 40 indirect) bytes in 10 blocks are definitely lost at 0x4849388: reallocarray by __sgr_dup (sgroupio.c:61) by update_groups (userdel.c:256) 200 (160 direct, 40 indirect) bytes in 10 blocks are definitely lost at 0x4849388: reallocarray by __sgr_dup (sgroupio.c:86) by update_groups (userdel.c:256) definitely lost: 480 bytes in 30 blocks after this commit: definitely lost: 0 bytes in 0 blocks Signed-off-by: Artem Semenov Reviewed-by: Alejandro Colomar --- lib/list.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/list.c b/lib/list.c index 4600d3a8e3..bafcd14618 100644 --- a/lib/list.c +++ b/lib/list.c @@ -128,6 +128,8 @@ del_list(/*@returned@*/ /*@only@*/char **list, const char *member) tmp[j] = NULL; + free (list); + return tmp; }