Bug 191427 - "pw userdel" can go into an infinite loop
Summary: "pw userdel" can go into an infinite loop
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 10.0-RELEASE
Hardware: Any Any
: Normal Affects Many People
Assignee: Devin Teske
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-26 22:46 UTC by yenbut
Modified: 2017-05-22 16:19 UTC (History)
3 users (show)

See Also:
ngie: mfc-stable8-
bugmeister: mfc-stable9?
ngie: mfc-stable10+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description yenbut 2014-06-26 22:46:10 UTC
There is an infinite loop in /usr/src/usr.sbin/pw/pw_user.c.
	Below is a code segment from pw_user.c:

           if (!strcmp(grp->gr_mem[i], a_name->val)) {
	     while (grp->gr_mem[i] != NULL) {
	       grp->gr_mem[i] = grp->gr_mem[i+1];
	     }
	     strlcpy(group, grp->gr_name, MAXLOGNAME);
	     chggrent(group, grp);
	   }

	Note that the index variable i in the while loop is static.
	If the content of gr_mem[i+1] being copied is not NULL, the
	while loop would run forever.

Environment:
System: FreeBSD bld017c1e.cs.washington.edu 10.0-RELEASE-p6 FreeBSD 10.0-RELEASE-p6 #0: Tue Jun 24 07:47:37 UTC 2014 root@amd64-builder.daemonology.net:/usr/obj/usr/src/sys/GENERIC amd64

How-To-Repeat:
It is obvious from the code.  However, to be certain, become
	root and run the following commands:

	pw useradd test
	pw groupmod test -M 'test,root'
	pw userdel test

	The last command would run forever. For cleaning up the 'test'
	group, hit Ctrl-C and then run the following command.

	pw groupdel test

Fix:
Here is one way to fix it:

--- pw_user.c	2014/06/26 21:39:49	1.1
+++ pw_user.c	2014/06/26 21:41:18
@@ -429,12 +429,14 @@
 				delgrent(GETGRNAM(a_name->val));
 			SETGRENT();
 			while ((grp = GETGRENT()) != NULL) {
-				int i;
+			        int i,j;
 				char group[MAXLOGNAME];
 				for (i = 0; grp->gr_mem[i] != NULL; i++) {
 					if (!strcmp(grp->gr_mem[i], a_name->val)) {
-						while (grp->gr_mem[i] != NULL) {
-							grp->gr_mem[i] = grp->gr_mem[i+1];
+					        j = i;
+						while (grp->gr_mem[j] != NULL) {
+							grp->gr_mem[j] = grp->gr_mem[j+1];
+							j += 1;
 						}	
 						strlcpy(group, grp->gr_name, MAXLOGNAME);
 						chggrent(group, grp);
Comment 1 Mateusz Guzik freebsd_committer 2014-06-26 23:16:12 UTC
I agree with problem statement and idea for the fix, but it can be done slightly prettier.

Care to rewrite the patch with a for loop instead?
Comment 2 commit-hook freebsd_committer 2014-06-27 18:51:57 UTC
A commit references this bug:

Author: mjg
Date: Fri Jun 27 18:51:19 UTC 2014
New revision: 267970
URL: http://svnweb.freebsd.org/changeset/base/267970

Log:
  pw: fix up deletion of users from groups

  Previuosly given 'foo,bar' members, removing 'foo' would result in an
  infinite loop.

  PR:		191427
  Submitted by:	Voradesh Yenbut <yenbut cs.washington.edu>
  MFC after:	1 week

Changes:
  head/usr.sbin/pw/pw_user.c
Comment 3 Mateusz Guzik freebsd_committer 2014-06-27 18:52:59 UTC
Committed, thanks.
Comment 4 Brad Davis freebsd_committer 2014-09-12 22:13:46 UTC
Can you MFC this before 10.1 please?
Comment 5 commit-hook freebsd_committer 2014-09-26 23:01:32 UTC
A commit references this bug:

Author: dteske
Date: Fri Sep 26 23:01:28 UTC 2014
New revision: 272192
URL: https://svnweb.freebsd.org/changeset/base/272192

Log:
  MFC revisions 262864-262865, 263114, 267970:
  r262864: Stop pw(8) from segfaulting when given certain input (julian)
  r262865: Part 2 of bug 187310 (julian)
  r263114: Fix pw(8) edge-case deletion of group "username" on userdel
  r267970: Fix infinite-loop during deletion of users from groups

  PR:		187310, 169471, 191427
  Submitted by:	Voradesh Yenbut, Alexander Pyhalov, Kim Shrier
  Obtained from:	bug
  Approved by:	re (gjb)

Changes:
_U  stable/10/
  stable/10/usr.sbin/pw/pw_group.c
  stable/10/usr.sbin/pw/pw_user.c
Comment 6 Ngie Cooper freebsd_committer 2015-10-25 21:58:17 UTC
The fixes haven't been committed to stable/9 yet, as noted in bug 169471 comment # 4. Reassigning to dteske for analysis. Please feel free to close this bug if you feel this commit shouldn't be MFCed to stable/9.
Comment 7 Devin Teske freebsd_committer 2015-10-26 19:57:58 UTC
Why is this assigned to me?