Bug 192837 - [patch] su(1) does not need to fork; it causes terminal problems
Summary: [patch] su(1) does not need to fork; it causes terminal problems
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 10.0-RELEASE
Hardware: amd64 Any
: --- Affects Some People
Assignee: freebsd-bugs (Nobody)
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-08-19 18:28 UTC by Kevin Barry
Modified: 2022-10-17 12:39 UTC (History)
3 users (show)

See Also:


Attachments
patch for the modification suggested above (2.33 KB, patch)
2014-08-19 18:28 UTC, Kevin Barry
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Barry 2014-08-19 18:28:17 UTC
Created attachment 146044 [details]
patch for the modification suggested above

Problem:

When calling /usr/bin/su, there are several unconditional calls to tcsetpgrp, which changes control of the terminal. This causes problems when the su call is a part of a pipeline and other processes in that pipeline require terminal access. For example, if I run the following:

root@host$ su -m nobody -c 'find /' | less

...less will get stuck in the background. This is a problem when the call to su is embedded in a script (e.g., root scripts that need to occasionally do something as a normal user), and that script is a part of a pipeline, because one can't simply move the rest of the pipeline into the su command.

The calls to tcsetpgrp are only necessary because su forks and creates a new process group for the child. Because the child potentially needs the terminal for authentication or executing the command, it needs terminal control, which takes it away from whatever process group su is a part of, e.g., a pipeline.


Solution:

I don't see a good reason for the fork+setpgid+waitpid code (https://svnweb.freebsd.org/base/stable/10/usr.bin/su/su.c?revision=256281&view=markup#l445). Really, the only thing it accomplishes is having the original suid process hang around until the command finishes, and it prevents the command from being a part of the pipeline it's embedded in. (e.g., in "su -m nobody -c 'find /' | less", "find" and "less" will not be in the same process group.) The fork code causes problems under these limited circumstances, without any apparent benefit. I therefore suggest that the fork code be removed, providing expected behavior to su. (Just for comparison, GNU su doesn't fork, and it exhibits the expected behavior.)
Comment 1 Kevin Barry 2014-08-19 18:30:29 UTC
Note that su.c is located at /usr/src/usr.bin/su/su.c.
Comment 2 Jilles Tjoelker freebsd_committer freebsd_triage 2015-05-05 21:58:00 UTC
The fork is required so that PAM may be shut down correctly.

The setpgid code is there for csh. See SVN r153985 and previous for details. Apparently csh does not make itself a process group leader when it is interactive and job control is enabled. Fixing this is harder than it seems.
Comment 3 Kevin Barry 2015-05-05 22:41:51 UTC
(In reply to Jilles Tjoelker from comment #2)

"The fork is required so that PAM may be shut down correctly."

This seems reasonable. I assume that the PAM shutdown needs to happen after the child exits?

"The setpgid code is there for csh. See SVN r153985 and previous for details. Apparently csh does not make itself a process group leader when it is interactive and job control is enabled. Fixing this is harder than it seems."

Is this a bug or a feature of csh? It seems a bit extreme to influence the behavior of su based on an idiosyncrasy of one of the countless possible commands that it can execute. It seems perfectly fine for csh to not become the process group leader in that situation, so is there really a need to force that to be the case when executing it from su? Would it break existing code/usage to: 1) remove all of the pg-related code in su; and/or 2) update csh to make it become process group leader if it isn't already the session leader?
Comment 4 Graham Perrin freebsd_committer freebsd_triage 2022-10-17 12:39:19 UTC
Keyword: 

    patch
or  patch-ready

– in lieu of summary line prefix: 

    [patch]

* bulk change for the keyword
* summary lines may be edited manually (not in bulk). 

Keyword descriptions and search interface: 

    <https://bugs.freebsd.org/bugzilla/describekeywords.cgi>