fix: memory leak of ngrp in updates_groups() by savoptik · Pull Request #1638 · shadow-maint/shadow

savoptik · 2026-06-08T14:14:19Z

In src/userdel.c:update_groups(), the 'ngrp' pointer was not freed
after use, leading to a memory leak. Added free(ngrp) call.

In src/userdel.c:update_groups(), the 'nsgrp' pointer was not freed
between loop iterations, leading to cumulative memory leaks. Added
free(nsgrp) call before proceeding to next iteration.

savoptik · 2026-08-03T12:49:38Z

ping

alejandro-colomar · 2026-08-03T13:15:00Z

ping

Thanks for the ping! I either missed this or forgot about it (I'm not sure which, since I had unusually high email traffic these days).

alejandro-colomar · 2026-08-06T12:51:49Z

Patches 1..4 are pretty trivial, and independent of the rest. I think it would be good if you opened a separate PR with those patches only, and then I'd merge it already. Then we can continue to discuss the remaining patches here.

savoptik · 2026-08-06T13:51:14Z

Patches 1..4 are pretty trivial, and independent of the rest. I think it would be good if you opened a separate PR with those patches only, and then I'd merge it already. Then we can continue to discuss the remaining patches here.

#1711

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 <savoptik@altlinux.org>

savoptik · 2026-08-07T12:03:41Z

I took the liberty of synchronizing the branch with master to avoid having too many differences. Could you please let me know if you have any further questions regarding these changes?

alejandro-colomar · 2026-08-08T00:09:44Z

I took the liberty of synchronizing the branch with master to avoid having too many differences.

Thanks!

Could you please let me know if you have any further questions regarding these changes?

I'll review tomorrow (I've been traveling today).

savoptik · 2026-08-08T09:12:19Z

I'll review tomorrow (I've been traveling today).

Have a great rest and an interesting journey!

alejandro-colomar · 2026-08-08T12:05:27Z

Thanks! The only unresolved issue is #1638 (comment).

alejandro-colomar · 2026-08-18T09:56:15Z

I'll review tomorrow (I've been traveling today).

Have a great rest and an interesting journey!

Thanks! I'm back from vacation. :)

Thanks! The only unresolved issue is #1638 (comment).

Please check this. Once we clarify that from the commit message, I'll merge.

@only

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 <savoptik@altlinux.org> Reviewed-by: Alejandro Colomar <alx@kernel.org>

@only

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 <savoptik@altlinux.org> Reviewed-by: Alejandro Colomar <alx@kernel.org>

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 <savoptik@altlinux.org> Reviewed-by: Alejandro Colomar <alx@kernel.org>

@only

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 <prefix> 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 <savoptik@altlinux.org> Reviewed-by: Alejandro Colomar <alx@kernel.org>

savoptik · 2026-08-20T12:45:27Z

I'll review tomorrow (I've been traveling today).

Have a great rest and an interesting journey!

Thanks! I'm back from vacation. :)

Thanks! The only unresolved issue is #1638 (comment).

Please check this. Once we clarify that from the commit message, I'll merge.

I updated the commit description.

alejandro-colomar

Thanks!

alejandro-colomar reviewed Jun 8, 2026

View reviewed changes

Comment thread src/userdel.c Outdated

alejandro-colomar mentioned this pull request Jun 8, 2026

fix: memory leak of nsgrp in src/useradd.c grp_update() #1639

Closed

savoptik force-pushed the savoptik/mlik-userdel branch from 13e9f5d to 26d9972 Compare June 9, 2026 11:34

alejandro-colomar reviewed Jun 9, 2026

View reviewed changes

Comment thread src/userdel.c

savoptik force-pushed the savoptik/mlik-userdel branch from 26d9972 to 15cc201 Compare July 15, 2026 09:37

alejandro-colomar reviewed Jul 20, 2026

View reviewed changes

Comment thread lib/list.c

savoptik force-pushed the savoptik/mlik-userdel branch 3 times, most recently from bf13d38 to c156b77 Compare July 24, 2026 12:49

alejandro-colomar reviewed Jul 27, 2026

View reviewed changes

Comment thread src/userdel.c

alejandro-colomar reviewed Jul 27, 2026

View reviewed changes

Comment thread src/useradd.c

alejandro-colomar reviewed Jul 27, 2026

View reviewed changes

Comment thread src/useradd.c

alejandro-colomar reviewed Jul 27, 2026

View reviewed changes

Comment thread src/userdel.c

alejandro-colomar reviewed Jul 27, 2026

View reviewed changes

Comment thread lib/list.c

savoptik force-pushed the savoptik/mlik-userdel branch from c156b77 to f71fdac Compare July 29, 2026 11:03