Bug 191427
| Summary: | "pw userdel" can go into an infinite loop | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Voradesh Yenbut <yenbut> |
| Component: | bin | Assignee: | Devin Teske <dteske> |
| Status: | Closed FIXED | ||
| Severity: | Affects Many People | CC: | brd, mjg, ngie |
| Priority: | Normal | Flags: | ngie:
mfc-stable10+
bugmeister: mfc-stable9? ngie: mfc-stable8- |
| Version: | 10.0-RELEASE | ||
| Hardware: | Any | ||
| OS: | Any | ||
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? 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 Committed, thanks. Can you MFC this before 10.1 please? 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 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. Why is this assigned to me? |
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);