src/groupmod.c: delete gr_free_members(&grp) to avoid double free

[gOo0se]

When ori_group have more than 3 users, try "groupmod -n new_group -U user1,user2 ori_group",core dump occured.

Reading the source code and discovering in grp_update, "grp = ogrp;" makes grp.gr_mem = ogrp.gr_mem, so if -U without -a, gr_free_members(&grp) would free *ogrp.gr_mem, which makes original group_db original entry.gr_mem become a wild pointer.

If no -n, gr_update (&grp) would replace ogrp with grp, nothing would happen; but if with -n, gr_update (&grp) would creat new entry so while write_all write ogrp.gr_mem ,core dump occurs.

Add !nflg to avoid this situation.

[alejandro-colomar]

What version of shadow (and OS) are you using? I can't reproduce this crash on my system.

$ sudo useradd -M -N user_foo_1
$ sudo useradd -M -N user_foo_2
$ sudo useradd -M -N user_foo_3
$ sudo useradd -M -N user_foo_4
$ sudo useradd -M -N user_foo_5
$ sudo groupadd -U user_foo_5 group_foo_1
$ grep group_foo /etc/group
group_foo_1:x:1004:user_foo_5
$ sudo groupmod -n group_foo_2 -U user_foo_1 group_foo_1
$ grep group_foo /etc/group
group_foo_2:x:1004:user_foo_1
$ sudo groupmod -n group_foo_2 -U user_foo_1,user_foo_2 group_foo_2
$ grep group_foo /etc/group
group_foo_2:x:1004:user_foo_1,user_foo_2
$ sudo groupmod -n group_foo_3 -U user_foo_1,user_foo_2,user_foo_3 group_foo_2
$ echo $?
0
$ grep group_foo /etc/group
group_foo_3:x:1004:user_foo_1,user_foo_2,user_foo_3
$ sudo groupmod -n group_foo_4 -U user_foo_1,user_foo_2,user_foo_3,user_foo_4 group_foo_3
$ echo $?
0
$ grep group_foo /etc/group
group_foo_4:x:1004:user_foo_1,user_foo_2,user_foo_3,user_foo_4

[gOo0se]

What version of shadow (and OS) are you using? I can't reproduce this crash on my system.

$ sudo useradd -M -N user_foo_1
$ sudo useradd -M -N user_foo_2
$ sudo useradd -M -N user_foo_3
$ sudo useradd -M -N user_foo_4
$ sudo useradd -M -N user_foo_5
$ sudo groupadd -U user_foo_5 group_foo_1
$ grep group_foo /etc/group
group_foo_1:x:1004:user_foo_5
$ sudo groupmod -n group_foo_2 -U user_foo_1 group_foo_1
$ grep group_foo /etc/group
group_foo_2:x:1004:user_foo_1
$ sudo groupmod -n group_foo_2 -U user_foo_1,user_foo_2 group_foo_2
$ grep group_foo /etc/group
group_foo_2:x:1004:user_foo_1,user_foo_2
$ sudo groupmod -n group_foo_3 -U user_foo_1,user_foo_2,user_foo_3 group_foo_2
$ echo $?
0
$ grep group_foo /etc/group
group_foo_3:x:1004:user_foo_1,user_foo_2,user_foo_3
$ sudo groupmod -n group_foo_4 -U user_foo_1,user_foo_2,user_foo_3,user_foo_4 group_foo_3
$ echo $?
0
$ grep group_foo /etc/group
group_foo_4:x:1004:user_foo_1,user_foo_2,user_foo_3,user_foo_4

I clone master branch and make it, reproduce this crash easily. How about trying it this way?

[root@localhost src]# ./useradd u1
[root@localhost src]# ./useradd u2
[root@localhost src]# ./useradd u3
[root@localhost src]# ./useradd u4
[root@localhost src]# ./groupadd -U u1,u2,u3,u4 g1
[root@localhost src]# ./groupmod -n g2 -U u1,u2 g1
Segmentation fault

[alejandro-colomar]

I clone master branch and make it, reproduce this crash easily. How about trying it this way?

[root@localhost src]# ./useradd u1
[root@localhost src]# ./useradd u2
[root@localhost src]# ./useradd u3
[root@localhost src]# ./useradd u4
[root@localhost src]# ./groupadd -U u1,u2,u3,u4 g1
[root@localhost src]# ./groupmod -n g2 -U u1,u2 g1
Segmentation fault

Yup, I can reproduce it on Debian (shadow 4.13) with that. Thanks!

[alejandro-colomar]

Now I'm wondering if usermod(8) will have a similar issue with -l and -G combined. The implementation is probably similar.

[gOo0se]

Now I'm wondering if usermod(8) will have a similar issue with -l and -G combined. The implementation is probably similar.

Great idea, I'll try this soon.

[alejandro-colomar]

I've traced the crash to this path:

https://github.com/shadow-maint/shadow/blob/69f74dbf8a2341aa73dc3f2213102eae1a3e49e0/src/groupmod.c#L857 https://github.com/shadow-maint/shadow/blob/69f74dbf8a2341aa73dc3f2213102eae1a3e49e0/src/groupmod.c#L470 https://github.com/shadow-maint/shadow/blob/69f74dbf8a2341aa73dc3f2213102eae1a3e49e0/lib/commonio.c#L981 https://github.com/shadow-maint/shadow/blob/69f74dbf8a2341aa73dc3f2213102eae1a3e49e0/lib/commonio.c#L879 https://github.com/shadow-maint/shadow/blob/69f74dbf8a2341aa73dc3f2213102eae1a3e49e0/lib/groupio.c#L64

It doesn't enter the valid_field() call, which makes me suspect that gr->gr_name is a free(3)d pointer, so having a reference to it is already illegal, and causes the crash.

(Edit: Or more likely, it's that gr is a freed pointer. That's more likely to cause a crash, than just passing a free(3)d pointer. Since it's reading *gr free(3)d memory for getting gr->gr_name, that's an easy crash.)

You can try yourself with this diff:

diff --git i/lib/commonio.c w/lib/commonio.c
index 01a26c96..bb01bba1 100644
--- i/lib/commonio.c
+++ w/lib/commonio.c
@@ -871,23 +871,35 @@ static int write_all (const struct commonio_db *db)
 {
        const struct commonio_entry *p;
        void *eptr;
+       int i = 0;

-       for (p = db->head; NULL != p; p = p->next) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+       for (p = db->head; NULL != p; p = p->next, i++) {
+fprintf(stderr, "ALX: %s():%d iter:%d\n", __func__, __LINE__, i); getchar();
                if (p->changed) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                        eptr = p->eptr;
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                        assert (NULL != eptr);
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                        if (db->ops->put (eptr, db->fp) != 0) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                                return -1;
                        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                } else if (NULL != p->line) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                        if (db->ops->fputs (p->line, db->fp) == EOF) {
                                return -1;
                        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                        if (putc ('\n', db->fp) == EOF) {
                                return -1;
                        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                }
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
        return 0;
 }

@@ -978,9 +990,12 @@ int commonio_close (struct commonio_db *db)
                goto fail;
        }

+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
        if (write_all (db) != 0) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                errors++;
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

        if (fflush (db->fp) != 0) {
                errors++;
diff --git i/lib/fields.c w/lib/fields.c
index 53929248..0e38438a 100644
--- i/lib/fields.c
+++ w/lib/fields.c
@@ -31,9 +31,11 @@ int valid_field (const char *field, const char *illegal)
        const char *cp;
        int err = 0;

+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
        if (NULL == field) {
                return -1;
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

        /* For each character of field, search if it appears in the list
         * of illegal characters. */
diff --git i/lib/groupio.c w/lib/groupio.c
index 7b9d45f2..8b6d73b2 100644
--- i/lib/groupio.c
+++ w/lib/groupio.c
@@ -60,15 +60,41 @@ static int group_put (const void *ent, FILE * file)
 {
        const struct group *gr = ent;

+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+       if (NULL == gr)
+{
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+               return -1;
+}
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+       if (valid_field(gr->gr_name, ":\n") == -1)
+{
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+               return -1;
+}
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+       if (valid_field(gr->gr_passwd, ":\n") == -1)
+{
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+               return -1;
+}
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+       if (gr->gr_gid == (gid_t)-1) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+               return -1;
+       }
        if (   (NULL == gr)
            || (valid_field (gr->gr_name, ":\n") == -1)
            || (valid_field (gr->gr_passwd, ":\n") == -1)
            || (gr->gr_gid == (gid_t)-1)) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                return -1;
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

        /* FIXME: fail also if gr->gr_mem == NULL ?*/
        if (NULL != gr->gr_mem) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                size_t i;
                for (i = 0; NULL != gr->gr_mem[i]; i++) {
                        if (valid_field (gr->gr_mem[i], ",:\n") == -1) {
@@ -76,6 +102,7 @@ static int group_put (const void *ent, FILE * file)
                        }
                }
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

        return (putgrent (gr, file) == -1) ? -1 : 0;
 }
diff --git i/lib/groupmem.c w/lib/groupmem.c
index 69d4435b..54bd0215 100644
--- i/lib/groupmem.c
+++ w/lib/groupmem.c
@@ -67,6 +67,7 @@

 void gr_free_members (struct group *grent)
 {
+fprintf(stderr, "ALX: %s() BEGIN\n", __func__); getchar();
        if (NULL != grent->gr_mem) {
                size_t i;
                for (i = 0; NULL != grent->gr_mem[i]; i++) {
@@ -75,11 +76,13 @@ void gr_free_members (struct group *grent)
                free (grent->gr_mem);
                grent->gr_mem = NULL;
        }
+fprintf(stderr, "ALX: %s() END\n", __func__); getchar();
 }

 void
 gr_free(/*@only@*/struct group *grent)
 {
+fprintf(stderr, "ALX: %s() BEGIN\n", __func__); getchar();
        free (grent->gr_name);
        if (NULL != grent->gr_passwd) {
                strzero (grent->gr_passwd);
@@ -87,4 +90,5 @@ gr_free(/*@only@*/struct group *grent)
        }
        gr_free_members(grent);
        free (grent);
+fprintf(stderr, "ALX: %s() END\n", __func__); getchar();
 }
diff --git i/src/groupmod.c w/src/groupmod.c
index a29cf73f..8c2f1df3 100644
--- i/src/groupmod.c
+++ w/src/groupmod.c
@@ -280,12 +280,15 @@ static void grp_update (void)
                         Prog, gr_dbname (), grp.gr_name);
                exit (E_GRP_UPDATE);
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
        if (nflg && (gr_remove (group_name) == 0)) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                fprintf (stderr,
                         _("%s: cannot remove entry '%s' from %s\n"),
                         Prog, grp.gr_name, gr_dbname ());
                exit (E_GRP_UPDATE);
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

 #ifdef SHADOWGRP
        /*
@@ -309,6 +312,7 @@ static void grp_update (void)
                }
        }
 #endif                         /* SHADOWGRP */
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
 }

 /*
@@ -467,12 +471,15 @@ static void process_flags (int argc, char **argv)
  */
 static void close_files (void)
 {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
        if (gr_close () == 0) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                fprintf (stderr,
                         _("%s: failure while writing changes to %s\n"),
                         Prog, gr_dbname ());
                exit (E_GRP_UPDATE);
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
 #ifdef WITH_AUDIT
        audit_logger (AUDIT_USER_ACCT, Prog,
                      info_group.audit_msg,
@@ -852,9 +859,12 @@ int main (int argc, char **argv)
         */
        open_files ();

+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
        grp_update ();
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

        close_files ();
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

        nscd_flush_cache ("group");
        sssd_flush_cache (SSSD_DB_GROUP);

Which shows the path where it crashes.

However, this is convoluted enough that I can't think of a fix. :|

[alejandro-colomar]

I've traced the crash to this path:

https://github.com/shadow-maint/shadow/blob/69f74dbf8a2341aa73dc3f2213102eae1a3e49e0/src/groupmod.c#L857

https://github.com/shadow-maint/shadow/blob/69f74dbf8a2341aa73dc3f2213102eae1a3e49e0/src/groupmod.c#L470

https://github.com/shadow-maint/shadow/blob/69f74dbf8a2341aa73dc3f2213102eae1a3e49e0/lib/commonio.c#L981

https://github.com/shadow-maint/shadow/blob/69f74dbf8a2341aa73dc3f2213102eae1a3e49e0/lib/commonio.c#L879

https://github.com/shadow-maint/shadow/blob/69f74dbf8a2341aa73dc3f2213102eae1a3e49e0/lib/groupio.c#L64

(Edit: Or more likely, it's that gr is a freed pointer. That's more likely to cause a crash, than just passing a free(3)d pointer. Since it's reading *gr free(3)d memory for getting gr->gr_name, that's an easy crash.)

[...]

However, this is convoluted enough that I can't think of a fix. :|

Maybe your fix is the right one (I don't know). But I don't know how to prove it. :)

[alejandro-colomar]

This bug is a use-after-free, not a double-free. (Or maybe could be an uninitialized value...)

[gOo0se]

This bug is a use-after-free, not a double-free. (Or maybe could be an uninitialized value...)

In my opinion, it is both use-after-free and double-free.

[root@localhost ~]# useradd u1
[root@localhost ~]# useradd u2
[root@localhost ~]# useradd u3
[root@localhost ~]# useradd u4
[root@localhost ~]# useradd u5
[root@localhost ~]# groupadd -U u1,u2,u3,u4,u5 g1
[root@localhost ~]# groupmod -n g2 -U u4,u3 g1
Segmentation fault

This test case obviously caused by double-free. You can try it.

[gOo0se]

I tracked two test cases using GDB, and the reason for the different crash results is shown in the following figure： This is a screenshot of the test case located in remove: the test case in the first image encountered an error in write_all, and it can be seen that the value of gr_mem [0] in remove is null, so it did not enter for and ended after releasing gr_mem without a crash; And the test case in the second image reported an error at remove, which is the double free I mentioned earlier. It can be seen that the value of gr_mem [0] in remove is already a wild pointer, so an error will be reported here

[gOo0se]

I clone master branch and make it, reproduce this crash easily. How about trying it this way?

[root@localhost src]# ./useradd u1
[root@localhost src]# ./useradd u2
[root@localhost src]# ./useradd u3
[root@localhost src]# ./useradd u4
[root@localhost src]# ./groupadd -U u1,u2,u3,u4 g1
[root@localhost src]# ./groupmod -n g2 -U u1,u2 g1
Segmentation fault

As for why the crash occurred in 'write_all', I also used gdb for tracking, and added some code to prevent memory reclamation, here is code:

diff -urN a/src/groupmod.c b/src/groupmod.c
--- a/src/groupmod.c    2024-06-05 06:16:44.176606315 +0800
+++ b/src/groupmod.c    2024-06-05 07:41:49.932606315 +0800
@@ -264,6 +264,14 @@
update_primary_groups (ogrp->gr_gid, group_newid);
}

• / test begin /
• const struct group *ogrp_0;
• ogrp_0 = gr_locate (group_name);
• printf("%s \n", (*ogrp_0).gr_mem[0]); //to avoid recycling
• / test finish /

• if (user_list) { char *token;

@@ -279,6 +287,19 @@ grp.gr_mem = dup_list (grp.gr_mem); }

• / test begin /
• const struct group *ogrp_1;
• ogrp_1 = gr_locate (group_name);
• // print would cause crash, remove can not change, so here(line-281) free (*ogrp).gr_mem wrong.
• //to avoid recycling
• if((ogrp_0).gr_mem == (ogrp_1).gr_mem){
• printf("same! /n");
• }
• / test finish /

• token = strtok(user_list, ",");
  while (token) {
      if (prefix_getpwnam (token) == NULL) {

So we can see like this:
![why write_all](https://github.com/shadow-maint/shadow/assets/59961967/34b52fe2-6b02-4233-90f4-3b455eebcb8b)
Before `gr_free_members (&grp)`，(*ogrp_0).gr_mem is normal，then after free, (*ogrp_1).gr_mem be freed, 'write_all' would have sth wrong with it.

[alejandro-colomar]

I tracked two test cases using GDB, and the reason for the different crash results is shown in the following figure：

What are the reproducers for the two test cases? I'd like to try the one that I haven't tried yet.

This is a screenshot of the test case located in remove: the test case in the first image encountered an error in write_all, and it can be seen that the value of gr_mem [0] in remove is null, so it did not enter for and ended after releasing gr_mem without a crash; And the test case in the second image reported an error at remove, which is the double free I mentioned earlier. It can be seen that the value of gr_mem [0] in remove is already a wild pointer, so an error will be reported here

The following diff does the following:

• Print every pointer before passing it to free(3) or our functions which free members.
• Print a line at some points to see the path, and stop at those with getchar(), to make sure that the compiler doesn't optimize them away.

diff --git i/lib/commonio.c w/lib/commonio.c
index 01a26c96..1e1668e7 100644
--- i/lib/commonio.c
+++ w/lib/commonio.c
@@ -331,6 +331,8 @@ static void free_linked_list (struct commonio_db *db)
                free (p->line);

                if (NULL != p->eptr) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: db->ops->free(p->eptr) = %p\n", p->eptr);
                        db->ops->free (p->eptr);
                }

@@ -707,6 +709,8 @@ int commonio_open (struct commonio_db *db, int mode)

       cleanup_entry:
        if (NULL != eptr) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: db->ops->free(p->eptr) = %p\n", p->eptr);
                db->ops->free (eptr);
        }
       cleanup_line:
@@ -871,14 +875,19 @@ static int write_all (const struct commonio_db *db)
 {
        const struct commonio_entry *p;
        void *eptr;
+       int i = 0;

-       for (p = db->head; NULL != p; p = p->next) {
+       for (p = db->head; NULL != p; p = p->next, i++) {
+fprintf(stderr, "ALX: %s():%d iter:%d\n", __func__, __LINE__, i); getchar();
                if (p->changed) {
                        eptr = p->eptr;
                        assert (NULL != eptr);
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                        if (db->ops->put (eptr, db->fp) != 0) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                                return -1;
                        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                } else if (NULL != p->line) {
                        if (db->ops->fputs (p->line, db->fp) == EOF) {
                                return -1;
@@ -978,9 +987,12 @@ int commonio_close (struct commonio_db *db)
                goto fail;
        }

+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
        if (write_all (db) != 0) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                errors++;
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

        if (fflush (db->fp) != 0) {
                errors++;
@@ -1069,9 +1081,13 @@ int commonio_update (struct commonio_db *db, const void *eptr)
        if (NULL != p) {
                if (next_entry_by_name (db, p->next, db->ops->getname (eptr)) != NULL) {
                        fprintf (shadow_logfd, _("Multiple entries named '%s' in %s. Please fix this with pwck or grpck.\n"), db->ops->getname (eptr), db->filename);
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: db->ops->free(p->eptr) = %p\n", nentry);
                        db->ops->free (nentry);
                        return 0;
                }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: db->ops->free(p->eptr) = %p\n", p->eptr);
                db->ops->free (p->eptr);
                p->eptr = nentry;
                p->changed = true;
@@ -1083,6 +1099,8 @@ int commonio_update (struct commonio_db *db, const void *eptr)
        /* not found, new entry */
        p = MALLOC(1, struct commonio_entry);
        if (NULL == p) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: db->ops->free(p->eptr) = %p\n", nentry);
                db->ops->free (nentry);
                errno = ENOMEM;
                return 0;
@@ -1120,6 +1138,8 @@ int commonio_append (struct commonio_db *db, const void *eptr)
        /* new entry */
        p = MALLOC(1, struct commonio_entry);
        if (NULL == p) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: db->ops->free(p->eptr) = %p\n", nentry);
                db->ops->free (nentry);
                errno = ENOMEM;
                return 0;
@@ -1182,6 +1202,8 @@ int commonio_remove (struct commonio_db *db, const char *name)
        free (p->line);

        if (NULL != p->eptr) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: db->ops->free(p->eptr) = %p\n", p->eptr);
                db->ops->free (p->eptr);
        }

diff --git i/lib/fields.c w/lib/fields.c
index 53929248..0e38438a 100644
--- i/lib/fields.c
+++ w/lib/fields.c
@@ -31,9 +31,11 @@ int valid_field (const char *field, const char *illegal)
        const char *cp;
        int err = 0;

+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
        if (NULL == field) {
                return -1;
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

        /* For each character of field, search if it appears in the list
         * of illegal characters. */
diff --git i/lib/groupio.c w/lib/groupio.c
index 7b9d45f2..c4d47baa 100644
--- i/lib/groupio.c
+++ w/lib/groupio.c
@@ -41,6 +41,7 @@ group_free(/*@only@*/void *ent)
 {
        struct group *gr = ent;

+fprintf(stderr, "ALX: gr_free(ent) = %p\n", ent);
        gr_free (gr);
 }

@@ -60,15 +61,44 @@ static int group_put (const void *ent, FILE * file)
 {
        const struct group *gr = ent;

+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: gr = %p)\n", gr);
+       if (NULL == gr)
+{
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+               return -1;
+}
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: gr->gr_name = %p\n", gr->gr_name);
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+       if (valid_field(gr->gr_name, ":\n") == -1)
+{
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+               return -1;
+}
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+       if (valid_field(gr->gr_passwd, ":\n") == -1)
+{
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+               return -1;
+}
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+       if (gr->gr_gid == (gid_t)-1) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+               return -1;
+       }
        if (   (NULL == gr)
            || (valid_field (gr->gr_name, ":\n") == -1)
            || (valid_field (gr->gr_passwd, ":\n") == -1)
            || (gr->gr_gid == (gid_t)-1)) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                return -1;
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

        /* FIXME: fail also if gr->gr_mem == NULL ?*/
        if (NULL != gr->gr_mem) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                size_t i;
                for (i = 0; NULL != gr->gr_mem[i]; i++) {
                        if (valid_field (gr->gr_mem[i], ",:\n") == -1) {
@@ -76,6 +106,7 @@ static int group_put (const void *ent, FILE * file)
                        }
                }
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

        return (putgrent (gr, file) == -1) ? -1 : 0;
 }
diff --git i/lib/groupmem.c w/lib/groupmem.c
index 69d4435b..bc10325a 100644
--- i/lib/groupmem.c
+++ w/lib/groupmem.c
@@ -33,6 +33,8 @@
        gr->gr_name = strdup (grent->gr_name);
        /*@=mustfreeonly@*/
        if (NULL == gr->gr_name) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: gr_free(gr) = %p\n", gr);
                gr_free(gr);
                return NULL;
        }
@@ -40,6 +42,8 @@
        gr->gr_passwd = strdup (grent->gr_passwd);
        /*@=mustfreeonly@*/
        if (NULL == gr->gr_passwd) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: gr_free(gr) = %p\n", gr);
                gr_free(gr);
                return NULL;
        }
@@ -50,12 +54,16 @@
        gr->gr_mem = MALLOC(i + 1, char *);
        /*@=mustfreeonly@*/
        if (NULL == gr->gr_mem) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: gr_free(gr) = %p\n", gr);
                gr_free(gr);
                return NULL;
        }
        for (i = 0; grent->gr_mem[i]; i++) {
                gr->gr_mem[i] = strdup (grent->gr_mem[i]);
                if (NULL == gr->gr_mem[i]) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: gr_free(gr) = %p\n", gr);
                        gr_free(gr);
                        return NULL;
                }
@@ -67,19 +75,24 @@

 void gr_free_members (struct group *grent)
 {
+fprintf(stderr, "ALX: %s() BEGIN\n", __func__); getchar();
        if (NULL != grent->gr_mem) {
                size_t i;
                for (i = 0; NULL != grent->gr_mem[i]; i++) {
+fprintf(stderr, "ALX: free(grent->gr_mem[%zu] = %p)\n", i, grent->gr_mem[i]);
                        free (grent->gr_mem[i]);
                }
+fprintf(stderr, "ALX: free(grent->gr_mem = %p)\n", grent->gr_mem[i]);
                free (grent->gr_mem);
                grent->gr_mem = NULL;
        }
+fprintf(stderr, "ALX: %s() END\n", __func__); getchar();
 }

 void
 gr_free(/*@only@*/struct group *grent)
 {
+fprintf(stderr, "ALX: %s() BEGIN\n", __func__); getchar();
        free (grent->gr_name);
        if (NULL != grent->gr_passwd) {
                strzero (grent->gr_passwd);
@@ -87,4 +100,5 @@ gr_free(/*@only@*/struct group *grent)
        }
        gr_free_members(grent);
        free (grent);
+fprintf(stderr, "ALX: %s() END\n", __func__); getchar();
 }
diff --git i/src/groupmod.c w/src/groupmod.c
index a29cf73f..8c2f1df3 100644
--- i/src/groupmod.c
+++ w/src/groupmod.c
@@ -280,12 +280,15 @@ static void grp_update (void)
                         Prog, gr_dbname (), grp.gr_name);
                exit (E_GRP_UPDATE);
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
        if (nflg && (gr_remove (group_name) == 0)) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                fprintf (stderr,
                         _("%s: cannot remove entry '%s' from %s\n"),
                         Prog, grp.gr_name, gr_dbname ());
                exit (E_GRP_UPDATE);
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

 #ifdef SHADOWGRP
        /*
@@ -309,6 +312,7 @@ static void grp_update (void)
                }
        }
 #endif                         /* SHADOWGRP */
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
 }

 /*
@@ -467,12 +471,15 @@ static void process_flags (int argc, char **argv)
  */
 static void close_files (void)
 {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
        if (gr_close () == 0) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
                fprintf (stderr,
                         _("%s: failure while writing changes to %s\n"),
                         Prog, gr_dbname ());
                exit (E_GRP_UPDATE);
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
 #ifdef WITH_AUDIT
        audit_logger (AUDIT_USER_ACCT, Prog,
                      info_group.audit_msg,
@@ -852,9 +859,12 @@ int main (int argc, char **argv)
         */
        open_files ();

+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
        grp_update ();
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

        close_files ();
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();

        nscd_flush_cache ("group");
        sssd_flush_cache (SSSD_DB_GROUP);

$ sudo cat /etc/group | grep g1
g1:x:1008:u1,u2,u3,u4
$ sudo ./src/groupmod -n g2 -U u1,u2 g1
ALX: main():862

ALX: gr_free_members() BEGIN

ALX: free(grent->gr_mem[0] = 0x555651950710)
ALX: free(grent->gr_mem[1] = 0x555651950730)
ALX: free(grent->gr_mem[2] = 0x555651950750)
ALX: free(grent->gr_mem[3] = 0x555651950770)
ALX: free(grent->gr_mem = (nil))
ALX: gr_free_members() END

ALX: grp_update():283

ALX: commonio_remove():1205

ALX: db->ops->free(p->eptr) = 0x555651950670
ALX: gr_free(ent) = 0x555651950670
ALX: gr_free() BEGIN

ALX: gr_free_members() BEGIN

ALX: free(grent->gr_mem = (nil))
ALX: gr_free_members() END

ALX: gr_free() END

ALX: grp_update():291

ALX: commonio_remove():1205

ALX: db->ops->free(p->eptr) = 0x555651957af0
ALX: grp_update():315

ALX: main():864

ALX: close_files():474

ALX: commonio_close():990

ALX: write_all():881 iter:0

ALX: write_all():881 iter:1

ALX: write_all():881 iter:2

ALX: write_all():881 iter:3

ALX: write_all():881 iter:4

ALX: write_all():881 iter:5

ALX: write_all():881 iter:6

ALX: write_all():881 iter:7

ALX: write_all():881 iter:8

ALX: write_all():881 iter:9

ALX: write_all():881 iter:10

ALX: write_all():881 iter:11

ALX: write_all():881 iter:12

ALX: write_all():881 iter:13

ALX: write_all():881 iter:14

ALX: write_all():881 iter:15

ALX: write_all():881 iter:16

ALX: write_all():881 iter:17

ALX: write_all():881 iter:18

ALX: write_all():881 iter:19

ALX: write_all():881 iter:20

ALX: write_all():881 iter:21

ALX: write_all():881 iter:22

ALX: write_all():881 iter:23

ALX: write_all():881 iter:24

ALX: write_all():881 iter:25

ALX: write_all():881 iter:26

ALX: write_all():881 iter:27

ALX: write_all():881 iter:28

ALX: write_all():881 iter:29

ALX: write_all():881 iter:30

ALX: write_all():881 iter:31

ALX: write_all():881 iter:32

ALX: write_all():881 iter:33

ALX: write_all():881 iter:34

ALX: write_all():881 iter:35

ALX: write_all():881 iter:36

ALX: write_all():881 iter:37

ALX: write_all():881 iter:38

ALX: write_all():881 iter:39

ALX: write_all():881 iter:40

ALX: write_all():881 iter:41

ALX: write_all():881 iter:42

ALX: write_all():881 iter:43

ALX: write_all():881 iter:44

ALX: write_all():881 iter:45

ALX: write_all():881 iter:46

ALX: write_all():881 iter:47

ALX: write_all():881 iter:48

ALX: write_all():881 iter:49

ALX: write_all():881 iter:50

ALX: write_all():881 iter:51

ALX: write_all():881 iter:52

ALX: write_all():881 iter:53

ALX: write_all():881 iter:54

ALX: write_all():881 iter:55

ALX: write_all():881 iter:56

ALX: write_all():881 iter:57

ALX: write_all():881 iter:58

ALX: write_all():881 iter:59

ALX: write_all():881 iter:60

ALX: write_all():881 iter:61

ALX: write_all():881 iter:62

ALX: write_all():881 iter:63

ALX: write_all():881 iter:64

ALX: write_all():881 iter:65

ALX: write_all():881 iter:66

ALX: write_all():881 iter:67

ALX: write_all():881 iter:68

ALX: write_all():881 iter:69

ALX: write_all():881 iter:70

ALX: write_all():881 iter:71

ALX: write_all():881 iter:72

ALX: write_all():881 iter:73

ALX: write_all():881 iter:74

ALX: write_all():885

ALX: group_put():64

ALX: gr = 0x4f4edfc4b4447e38)
ALX: group_put():71

Segmentation fault

I don't see any pointer value being passed twice to free(3).

The crash occurs consistently here:

--- i/lib/groupio.c
+++ w/lib/groupio.c
@@ -41,6 +41,7 @@ group_free(/*@only@*/void *ent)
 {
        struct group *gr = ent;

+fprintf(stderr, "ALX: gr_free(ent) = %p\n", ent);
        gr_free (gr);
 }

@@ -60,15 +61,44 @@ static int group_put (const void *ent, FILE * file)
 {
        const struct group *gr = ent;

+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: gr = %p)\n", gr);
+       if (NULL == gr)
+{
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+               return -1;
+}
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+fprintf(stderr, "ALX: gr->gr_name = %p\n", gr->gr_name);
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+       if (valid_field(gr->gr_name, ":\n") == -1)
+{
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+               return -1;
+}
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+       if (valid_field(gr->gr_passwd, ":\n") == -1)
+{
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+               return -1;
+}
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+       if (gr->gr_gid == (gid_t)-1) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__); getchar();
+               return -1;
+       }

and more precisely, here:

+fprintf(stderr, "ALX: gr->gr_name = %p\n", gr->gr_name);

The reason is dereferencing an invalid pointer. Why is it an invalid pointer? Maybe it was free(3)d before (but I can't find when; maybe an optimization changed its value; maybe it was stored in free(3)d memory and it has changed due to it being overwritten); maybe it was never initialized. Also, it doesn't even look like a pointer value, since other pointers seem to use just 12 hex digits, but this value has all 16 hex digits, so it seems garbagge (even with -O0 I end up with garbagge there).

If there's a double-free, we should be able to see a pointer value being passed to free(3) twice, I expect.

[alejandro-colomar]

Ahhhh, sorry, I've now been able to reproduce the double free.

I've reproduced it with

$ sudo cat /etc/group | grep g2
g2:x:1008:u1,u2,u3
$ sudo ./src/groupmod -n g2 -U u1,u2 g2
ALX: main():864

ALX: gr_free_members(ogrp) = 0x5611b0cf8bb0

ALX: grp_update():254

ALX: gr_free_members() BEGIN

ALX: grent = 0x7ffe2bfff510
ALX: grent->gr_mem = 0x5611b0cf8c20
ALX: free(grent->gr_mem[0] = 0x5611b0cf8c50)
ALX: gr_free_members():85

ALX: free(grent->gr_mem[1] = 0x5611b0cf8c70)
ALX: gr_free_members():85

ALX: free(grent->gr_mem[2] = 0x5611b0cf8c90)
ALX: gr_free_members():85

ALX: free(grent->gr_mem = (nil))
ALX: gr_free_members():89

ALX: grent = 0x7ffe2bfff510
ALX: grent->gr_mem = (nil)
ALX: gr_free_members() END

ALX: commonio_update():1089

ALX: db->ops->free(p->eptr) = 0x5611b0cf8bb0
ALX: gr_free(ent) = 0x5611b0cf8bb0
ALX: gr_free() BEGIN

ALX: gr_free_members() BEGIN

ALX: grent = 0x5611b0cf8bb0
ALX: grent->gr_mem = 0x5611b0cf8c20
ALX: free(grent->gr_mem[0] = 0x5611b0cf8)
ALX: gr_free_members():85

Segmentation fault

(and a few more fprintf(3) lines.)

And now I see the reason. Your commit message seems correct, that the copy grp = *ogrp; is the fault. We're copying the pointers and freeing the new copy, but keeping the old one around.

But why doesn't it always double-free? That's very weird. It should be; I think.

[gOo0se]

But why doesn't it always double-free? That's very weird. It should be; I think.

Yes, that is what I wander so I pull this code to see if maintainer have some ideas about it :D

[alejandro-colomar]

I managed to get a crash with just -U. No need for -n.

alx@debian:~/src/shadow/shadow/master$ sudo cat /etc/group | grep g2
g2:x:1008:u1,u2,u3
alx@debian:~/src/shadow/shadow/master$ sudo ./src/groupmod -U u1,u2 g2
ALX: grp_update():217
ALX: grp.gr_mem = 0x55b68622bc20
ALX: ogrp->gr_mem = 0x55b68622bc20

ALX: grp_update():252
ALX: grp.gr_mem = 0x55b68622bc20
ALX: ogrp->gr_mem = 0x55b68622bc20

ALX: grp_update():263
ALX: grp.gr_mem = (nil)
ALX: ogrp->gr_mem = 0x55b68622bc20

ALX: grp_update():269
ALX: grp.gr_mem = 0x55b68622bc90
ALX: ogrp->gr_mem = 0x55b68622bc20

ALX: grp_update():286
ALX: grp.gr_mem = 0x55b68622bc70
ALX: ogrp->gr_mem = 0x55b68622bc20

ALX: grp_update():286
ALX: grp.gr_mem = 0x55b686226550
ALX: ogrp->gr_mem = 0x55b68622bc20

ALX: grp_update():293
ALX: grp.gr_mem = 0x55b686226550
ALX: ogrp->gr_mem = 0x55b68622bc20

Segmentation fault

Here's the diff for analyzing the crash:

diff --git i/src/groupmod.c w/src/groupmod.c
index a29cf73f..900d6607 100644
--- i/src/groupmod.c
+++ w/src/groupmod.c
@@ -214,6 +214,10 @@ static void grp_update (void)
                exit (E_GRP_UPDATE);
        }
        grp = *ogrp;
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__);
+fprintf(stderr, "ALX: grp.gr_mem = %p\n", grp.gr_mem);
+fprintf(stderr, "ALX: ogrp->gr_mem = %p\n", ogrp->gr_mem);
+getchar();
        new_grent (&grp);
 #ifdef SHADOWGRP
        if (   is_shadow_grp
@@ -245,6 +249,10 @@ static void grp_update (void)
                update_primary_groups (ogrp->gr_gid, group_newid);
        }

+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__);
+fprintf(stderr, "ALX: grp.gr_mem = %p\n", grp.gr_mem);
+fprintf(stderr, "ALX: ogrp->gr_mem = %p\n", ogrp->gr_mem);
+getchar();
        if (user_list) {
                char *token;

@@ -252,8 +260,16 @@ static void grp_update (void)
                        // requested to replace the existing groups
                        if (NULL != grp.gr_mem[0])
                                gr_free_members(&grp);
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__);
+fprintf(stderr, "ALX: grp.gr_mem = %p\n", grp.gr_mem);
+fprintf(stderr, "ALX: ogrp->gr_mem = %p\n", ogrp->gr_mem);
+getchar();
                        grp.gr_mem = XMALLOC(1, char *);
                        grp.gr_mem[0] = NULL;
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__);
+fprintf(stderr, "ALX: grp.gr_mem = %p\n", grp.gr_mem);
+fprintf(stderr, "ALX: ogrp->gr_mem = %p\n", ogrp->gr_mem);
+getchar();
                } else {
                        // append to existing groups
                        if (NULL != grp.gr_mem[0])
@@ -267,25 +283,49 @@ static void grp_update (void)
                                exit (E_GRP_UPDATE);
                        }
                        grp.gr_mem = add_list(grp.gr_mem, token);
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__);
+fprintf(stderr, "ALX: grp.gr_mem = %p\n", grp.gr_mem);
+fprintf(stderr, "ALX: ogrp->gr_mem = %p\n", ogrp->gr_mem);
+getchar();
                        token = strtok(NULL, ",");
                }
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__);
+fprintf(stderr, "ALX: grp.gr_mem = %p\n", grp.gr_mem);
+fprintf(stderr, "ALX: ogrp->gr_mem = %p\n", ogrp->gr_mem);
+getchar();

        /*
         * Write out the new group file entry.
         */
        if (gr_update (&grp) == 0) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__);
+fprintf(stderr, "ALX: grp.gr_mem = %p\n", grp.gr_mem);
+fprintf(stderr, "ALX: ogrp->gr_mem = %p\n", ogrp->gr_mem);
+getchar();
                fprintf (stderr,
                         _("%s: failed to prepare the new %s entry '%s'\n"),
                         Prog, gr_dbname (), grp.gr_name);
                exit (E_GRP_UPDATE);
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__);
+fprintf(stderr, "ALX: grp.gr_mem = %p\n", grp.gr_mem);
+fprintf(stderr, "ALX: ogrp->gr_mem = %p\n", ogrp->gr_mem);
+getchar();
        if (nflg && (gr_remove (group_name) == 0)) {
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__);
+fprintf(stderr, "ALX: grp.gr_mem = %p\n", grp.gr_mem);
+fprintf(stderr, "ALX: ogrp->gr_mem = %p\n", ogrp->gr_mem);
+getchar();
                fprintf (stderr,
                         _("%s: cannot remove entry '%s' from %s\n"),
                         Prog, grp.gr_name, gr_dbname ());
                exit (E_GRP_UPDATE);
        }
+fprintf(stderr, "ALX: %s():%d\n", __func__, __LINE__);
+fprintf(stderr, "ALX: grp.gr_mem = %p\n", grp.gr_mem);
+fprintf(stderr, "ALX: ogrp->gr_mem = %p\n", ogrp->gr_mem);
+getchar();

 #ifdef SHADOWGRP
        /*

So, we have something very broken here.

(BTW, this is also reproducible on 4.13:)

$ sudo groupmod -U u1,u2 g2
Segmentation fault

[alejandro-colomar]

@hallyn , we might very well have something serious here. I don't even know where this should be fixed. Wild memory is being used. At least this one is not setuid, which mitigates the danger.

@gOo0se

But why doesn't it always double-free? That's very weird. It should be; I think.

Yes, that is what I wander so I pull this code to see if maintainer have some ideas about it :D

Thanks a lot for the report! It's been quite useful. Even if the bug is slightly different than the originally reported one, it helps investigate. :-)

[gOo0se]

I managed to get a crash with just -U. No need for -n.

I also reproduce this and my pull can not help it sadly.（Due to the time difference, I will continue watching tomorrow）

[gOo0se]

I have identified the area where groupmod -U u1,u2 g2 reported an error, which is also a problem with double free. However, the new second free is located in gr_update

First is still here in groupmod.c :

if (!aflg) { 
    // requested to replace the existing groups
    if (NULL != grp.gr_mem[0])
        gr_free_members(&grp);             // first free

Then the second :

/*
* Write out the new group file entry.
*/
if (gr_update (&grp) == 0) {                  // second free
fprintf (stderr,
               _("%s: failed to prepare the new %s entry '%s'\n"),
               Prog, gr_dbname (), grp.gr_name);
exit (E_GRP_UPDATE);
}

which calls

int gr_update (const struct group *gr)
{
    return commonio_update (&group_db, (const void *) gr);
}

which calls

int commonio_update (struct commonio_db *db, const void *eptr)
{
    struct commonio_entry *p;
    void *nentry;

    if (!db->isopen || db->readonly) {
        errno = EINVAL;
        return 0;
    }
    nentry = db->ops->dup (eptr);
    if (NULL == nentry) {
        errno = ENOMEM;
        return 0;
    }
    p = find_entry_by_name (db, db->ops->getname (eptr));
    if (NULL != p) {
        if (next_entry_by_name (db, p->next, db->ops->getname (eptr)) != NULL) {
            fprintf (shadow_logfd, _("Multiple entries named '%s' in %s. Please fix this with pwck or grpck.\n"), db->ops->getname (eptr), db->filename);
            db->ops->free (nentry);
            return 0;
        }
        db->ops->free (p->eptr);                      // core dump here
        p->eptr = nentry;
        p->changed = true;
        db->cursor = p;

which calls

static void group_free (/*@out@*/ /*@only@*/void *ent)
{
    struct group *gr = ent;

    gr_free (gr);
}

which calls

void gr_free (/*@out@*/ /*@only@*/struct group *grent)
{
    free (grent->gr_name);
    if (NULL != grent->gr_passwd) {
        memzero (grent->gr_passwd, strlen (grent->gr_passwd));
        free (grent->gr_passwd);
    }
    gr_free_members(grent);

which calls

void gr_free_members (struct group *grent)
{
    if (NULL != grent->gr_mem) {
        size_t i;
        for (i = 0; NULL != grent->gr_mem[i]; i++) {    
            free (grent->gr_mem[i]);
        }
        free (grent->gr_mem);
        grent->gr_mem = NULL;
    }
}

We can see grent->gr_mem[0] here is a wild pointer so it crashed， the screenshot is as follows :

[gOo0se]

But why doesn't it always double-free? That's very weird. It should be; I think.

Yes, that is what I wander so I pull this code to see if maintainer have some ideas about it :D

Auh, I have tracked the test cases for normal use of - U just like

[root@localhost ~]# cat /etc/group | grep g5
g5:x:1012:u1
[root@localhost ~]# groupmod -U u1,u2 g5

It is also called gr_update but nothing wrong with it . It is weird so I tracked it and then I found the same question : why grent->gr_mem[0] is 0x0 instead of wild pointer ?

I have tested many cases, but not all users will necessarily report errors if they decrease

[alejandro-colomar]

Please upload your new patch to this PR. I'll paste it here for the moment for commenting it.

diff --git a/src/groupmod.c b/src/groupmod.c
index a29cf73f6..989d7ea34 100644
--- a/src/groupmod.c
+++ b/src/groupmod.c
@@ -250,8 +250,6 @@ static void grp_update (void)

        if (!aflg) {
            // requested to replace the existing groups
-           if (NULL != grp.gr_mem[0])
-               gr_free_members(&grp);
            grp.gr_mem = XMALLOC(1, char *);
            grp.gr_mem[0] = NULL;
        } else {

That's one possibility. @hallyn suggests the same patch, so maybe that's it.

Before seeing that suggestion, I had thought that maybe we need to free it, but from the actual ogrp, not &grp, so my proposal would be:

diff --git i/src/groupmod.c w/src/groupmod.c
index a29cf73f..9cc2b3e2 100644
--- i/src/groupmod.c
+++ w/src/groupmod.c
@@ -250,8 +250,8 @@ static void grp_update (void)

        if (!aflg) {
            // requested to replace the existing groups
-           if (NULL != grp.gr_mem[0])
-               gr_free_members(&grp);
+           if (NULL != ogrp->gr_mem[0])
+               gr_free_members(ogrp);
            grp.gr_mem = XMALLOC(1, char *);
            grp.gr_mem[0] = NULL;
        } else {

I have no idea what's more appropriate. Anyone?

(also, and unrelated, but the if (NULL != grp.gr_mem[0]) is unnecessary because gr_free_members() just behaves well if it's NULL, so I'd remove it in a second patch.)

[gOo0se]

Before seeing that suggestion, I had thought that maybe we need to free it, but from the actual ogrp, not &grp

Maybe (*ogrp).gr_mem should not be freed ; I think.

I just tried your modify and it looks like this modification should also cause (* ogrp). gr_mem's double free, but becaues of the correctly free, grent->gr_mem[0] is 0x0 so no crash here.

[alejandro-colomar]

Before seeing that suggestion, I had thought that maybe we need to free it, but from the actual ogrp, not &grp

Maybe (*ogrp).gr_mem should not be freed ; I think.

I just tried your modify and it looks like this modification should also cause (* ogrp). gr_mem's double free, but becaues of the correctly free, grent->gr_mem[0] is 0x0 so no crash here.

It shouldn't cause a double-free, since gr_free_members() sets the members to NULL after freeing, so there wouldn't be wild pointers. The pointers would be set to NULL, which can be free()d again.

(Whoops; initially I accidentally edited your message instead of replying.)

[gOo0se]

Before seeing that suggestion, I had thought that maybe we need to free it, but from the actual ogrp, not &grp

Maybe (*ogrp).gr_mem should not be freed ; I think.

I just tried your modify and it looks like this modification should also cause (* ogrp). gr_mem's double free, but becaues of the correctly free, grent->gr_mem[0] is 0x0 so no crash here.

And I also try to modify grp=* ogrp, but if it does, memory needs to be freed within new_grp, this actually seems a bit troublesome.

[alejandro-colomar]

My patch seems to avoid the crash:

$ sudo cat /etc/group | grep g2
g2:x:1008:u1,u2,u3
$ sudo ./src/groupmod -U u1,u2 g2
$ sudo cat /etc/group | grep g2
g2:x:1008:u1,u2

I don't know if it has other problems, though.

[gOo0se]

My patch seems to avoid the crash:

$ sudo cat /etc/group | grep g2
g2:x:1008:u1,u2,u3
$ sudo ./src/groupmod -U u1,u2 g2
$ sudo cat /etc/group | grep g2
g2:x:1008:u1,u2

I don't know if it has other problems, though.

Yes, I've tried it, everything is ok. But I have a question, why do we need to free (*ogrp).gr_mem ?

[alejandro-colomar]

My patch seems to avoid the crash:

$ sudo cat /etc/group | grep g2
g2:x:1008:u1,u2,u3
$ sudo ./src/groupmod -U u1,u2 g2
$ sudo cat /etc/group | grep g2
g2:x:1008:u1,u2

I don't know if it has other problems, though.

Yes, I've tried it, everything is ok. But I have a question, why do we need to free (*ogrp).gr_mem ?

I don't know. I just thought that since someone wrote that gr_free_members() call, it thought something would need to be freed, and accidentally passed the wrong thing.

Maybe it's unnecessary. Since I don't understand how commonio works, I can't answer your question. :-)

[alejandro-colomar]

My patch seems to avoid the crash:

$ sudo cat /etc/group | grep g2
g2:x:1008:u1,u2,u3
$ sudo ./src/groupmod -U u1,u2 g2
$ sudo cat /etc/group | grep g2
g2:x:1008:u1,u2

I don't know if it has other problems, though.

Yes, I've tried it, everything is ok. But I have a question, why do we need to free (*ogrp).gr_mem ?

I don't know. I just thought that since someone wrote that gr_free_members() call, it thought something would need to be freed, and accidentally passed the wrong thing.

Maybe it's unnecessary. Since I don't understand how commonio works, I can't answer your question. :-)

On the other hand, since the other path (if aflg == true) doesn't free anything, maybe we don't need to free anything here either.

In either case, please add the following to your commit:

Fixes: 342c934a3590 ("add -U option to groupadd and groupmod")
Closes: <https://github.com/shadow-maint/shadow/issues/1013>
Link: <https://github.com/shadow-maint/shadow/pull/1007>
Link: <https://github.com/shadow-maint/shadow/pull/271>
Link: <https://github.com/shadow-maint/shadow/issues/265>
Cc: "Serge E. Hallyn" <serge@hallyn.com>

[gOo0se]

I think I agree with this change.

Please squash both commits into one, and add

Reviewed-by: Alejandro Colomar <alx@kernel.org>

uh, (sorry but) should I pull the code locally? I only know how to modify commit information locally and squash，then how should I submit this modification to this PR?

[alejandro-colomar]

I think I agree with this change. Please squash both commits into one, and add

Reviewed-by: Alejandro Colomar <alx@kernel.org>

uh, (sorry but) should I pull the code locally? I only know how to modify commit information locally and squash，then how should I submit this modification to this PR?

That's for the commit message. So, at the end of the commit message, write:

Fixes: 342c934a3590 ("add -U option to groupadd and groupmod")
Closes: <https://github.com/shadow-maint/shadow/issues/1013>
Link: <https://github.com/shadow-maint/shadow/pull/1007>
Link: <https://github.com/shadow-maint/shadow/pull/271>
Link: <https://github.com/shadow-maint/shadow/issues/265>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>

See for example: https://github.com/shadow-maint/shadow/commit/a6eb312f60a5c0e11483ead2fb1635f282da8e59 and https://github.com/shadow-maint/shadow/commit/71e28359d12491727b2e94c71d2e1e1682d45a02

See also: https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed%20off#sign-your-work-the-developer-s-certificate-of-origin

[alejandro-colomar]

(If you still find it difficult, please squash, and I'll handle the rest.)

[gOo0se]

(If you still find it difficult, please squash, and I'll handle the rest.)

Thanks, your suggestion helps a lot.

[alejandro-colomar]

Just a small detail:

Signed-off-by: ‘lixinyun’ <‘li.xinyun@h3c.com’>

The quotes in the email are invalid. See url(7). Everything inside the <> delimiters is the actual email. So your email should be spelled as <li.xinyun@h3c.com>.

The quotes in the name are also unnecessary, but not critical. I use quotes in Serge's name because it contains a ., but if there are only alphanumeric characters and spaces, you can leave it unquoted.

---

Edit:

Sorry, ignore the above. GitHub shows quotes for some broken reason, but your patch is good; it doesn't contain the quotes. GitHub is broken as always.

[alejandro-colomar]

LGTM; I'll merge now. @hallyn also agrees with the patch, from a private discussion we had.

Thanks!!