Bug 211195 - pw userdel Segmentation fault (core dumped)
Summary: pw userdel Segmentation fault (core dumped)
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 10.3-STABLE
Hardware: amd64 Any
: --- Affects Many People
Assignee: freebsd-bugs mailing list
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2016-07-18 12:02 UTC by eniorm
Modified: 2016-07-24 08:21 UTC (History)
4 users (show)

See Also:


Attachments
Patch for 211195 (350 bytes, patch)
2016-07-20 21:58 UTC, rday
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description eniorm 2016-07-18 12:02:16 UTC
Hello all

This is the first time I saw a bug in pw command.

I ran this command: pw user del username -r

For few users, and only one reported this error: Segmentation fault (core dumped)

But the username was removed correctly. 

System:
FreeBSD servidor.teste.net 10.3-STABLE FreeBSD 10.3-STABLE #0 r301006: Mon May 30 22:05:33 BRT 2016     root@samba.teste.net:/usr/obj/usr/src/sys/SAMBA  amd64

Rerun the steps

# pw user add someuser -g somegroup -d "/dev/null" -s "/usr/sbin/nologin"

# pw user del someuser -r
Segmentation fault (core dumped)

# pw user show someuser
pw: no such user 'someuser'


The pw.core dump core file cannot be attached because it too large (8,5M)

thanks
Comment 1 Ruslan Makhmatkhanov freebsd_committer 2016-07-19 06:59:28 UTC
I can reproduce that. It only triggered if pw user del invoked with -r flag against user, who have no home directory. If you first add user with -m flag (create home directory), then invoking user del with -r will success.

This is not reproducible on -head.

Adding bapt@, because he may know what the problem is.
Comment 2 eniorm 2016-07-19 11:42:39 UTC
Thats correct. I saw now and understood. 
How can -r flag to remove home if users has no home dir :D
Thanks
Comment 3 rday 2016-07-20 21:58:51 UTC
Created attachment 172760 [details]
Patch for 211195

The crash occurs in the rm_r() function, however the core dump issue looks a little more subtle than not having a home directory. For example, the commands

# pw user add someuser -g somegroup -d "/home/someuser" -s "/usr/sbin/nologin"
# pw user del someuser -r

Won't core dump. /home/someuser doesn't exist, and the problematic code never runs.

In your example the home directory was "/dev/null", which *does* exist even though -m wasn't specified. The program uses openat(2) with the O_DIRECTORY flag to open "/dev/null" which is not a directory. openat() returns an unchecked error, and the program crashes when it tries to open the invalid descriptor.

I was able to reproduce this in the master branch on the Github repo. I attached a patch for the rm_r() function to check the return value of openat(). It looks like openat()'s return value isn't checked in a couple other locations in the code as well. Those code paths may not be accessible though.
Comment 4 eniorm 2016-07-21 16:45:36 UTC
thanks for help and explainations :-)
Comment 5 commit-hook freebsd_committer 2016-07-23 10:20:05 UTC
A commit references this bug:

Author: bapt
Date: Sat Jul 23 10:19:10 UTC 2016
New revision: 303217
URL: https://svnweb.freebsd.org/changeset/base/303217

Log:
  Do not try to delete the home of the user if is is not a directory for example
  "/dev/null"

  PR:		211195
  Submitted by:	rday <ryan@ryanday.net>
  Reported by:	eniorm <eniorm@gmail.com>
  MFC after:	1 day

Changes:
  head/usr.sbin/pw/rm_r.c
  head/usr.sbin/pw/tests/pw_userdel.sh
Comment 6 Baptiste Daroussin freebsd_committer 2016-07-23 10:39:14 UTC
Thanks for reporting, I have committed the patch along with a regression test as r303217 I will merge it for 11.0-RELEASE
Comment 7 commit-hook freebsd_committer 2016-07-24 08:12:36 UTC
A commit references this bug:

Author: bapt
Date: Sun Jul 24 08:12:23 UTC 2016
New revision: 303256
URL: https://svnweb.freebsd.org/changeset/base/303256

Log:
  iDo not try to delete the home of the user if is is not a directory for example
  "/dev/null"

  PR:		211195
  Submitted by:	rday <ryan@ryanday.net>
  Reported by:	eniorm <eniorm@gmail.com>
  Approved by:	re (kib)

Changes:
_U  stable/11/
  stable/11/usr.sbin/pw/rm_r.c
  stable/11/usr.sbin/pw/tests/pw_userdel.sh
Comment 8 commit-hook freebsd_committer 2016-07-24 08:21:39 UTC
A commit references this bug:

Author: bapt
Date: Sun Jul 24 08:21:21 UTC 2016
New revision: 303257
URL: https://svnweb.freebsd.org/changeset/base/303257

Log:
  Do not try to delete the home of the user if is is not a directory for example
  "/dev/null"

  PR:		211195
  Submitted by:	rday <ryan@ryanday.net>
  Reported by:	eniorm <eniorm@gmail.com>

Changes:
_U  stable/10/
  stable/10/usr.sbin/pw/rm_r.c
  stable/10/usr.sbin/pw/tests/pw_userdel.sh