Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstream into tmux #17

Open
rking opened this issue Jan 25, 2013 · 3 comments
Open

Upstream into tmux #17

rking opened this issue Jan 25, 2013 · 3 comments

Comments

@rking
Copy link

rking commented Jan 25, 2013

I talked with NicM on IRC about htis, and he said:

"I think they tried to fix it in the port before but it broke things. But if
someone sends a patch that does it cleanly and safely under #ifdef then we
could include it in the portable tmux."

Do you want to collaborate on coming up with such a patch?

@ChrisJohnsen
Copy link
Owner

Maybe I am being overly conservative about this, but I am personally not comfortable advocating for the addition of an undocumented, unsupported API call to upstream tmux.

It could certainly be made safe for all non–OS X platforms (well actually, probably just non-Darwin). All the ugliness could be hidden in one spot (e.g. a new “osdep” function). But trying to make it safe across various OS X releases does impose a certain amount of ugliness:

  • We need to probe for the OS release number because the undocumented function gained an extra argument in 10.6, so there is a difference in how we should call the function on 10.5 and on 10.6–10.8. Future releases may need something else entirely.
  • We need to use dlsym to access the function instead of linking directly to it in case the undocumented function disappears in some future OS release.

Essentially, the wrapper’s code already does everything in the safest way I could think of. A patch to tmux would take most of the wrapper code (i.e. everything except the argument handling and the execvp(3) call), adapt it to the tmux style and suite of utility functions, then insert it somewhere after the tmux server calls daemon(3).

If we had any kind of reassurance from Apple that the method used in the wrapper was even semi-supported, then I would think it reasonable to patch this into tmux. But, everything I have seen from Apple indicates that they would prefer everyone move away from Unix-y methods of making background processes (e.g. the Daemons and Agents Technical Note, that daemon(3) is deprecated, and a comment next to the private header that declares the undocumented function cryptically says “One day, we'll be able to get rid of this...”), so it seems unlikely that they would bless this technique.

There is no doubt that integrating this into tmux (or even just a patch applied by MacPorts, Homebrew, etc.) would be nicer for users. On the other hand, if the technique were to catastrophically break in the future (e.g. calling the undocumented function causes a crash), it would be much easier for users to reset or edit their default-command (or whatever they use to invoke the wrapper) rather than have to wait for upstream tmux (or downstream Macports, et al.) to come up with and release a mitigating patch.

@rking
Copy link
Author

rking commented Jan 25, 2013

@ChrisJohnsen - Thanks for your response (and for your in-depth knowledge of the topic!)

Do you know what part of daemon() starts in with the odd namespace stuff? I was looking at http://opensource.apple.com/source/Libc/Libc-763.13/gen/FreeBSD/daemon.c and it looks pretty simple... some piece of it must be responsible.

I'm wondering if we can just call the pieces directly in tmux instead of letting this happen (but I don't understand the issues well enough to really be useful at this point).

@ChrisJohnsen
Copy link
Owner

The problematic bit in that 10.7 version of daemon(3) is the move_to_root_bootstrap function that is added by the patch that is applied to the gen/daemon.c file that you referenced. In 10.8 there is no gen/daemon.c, just an already patched gen/FreeBSD/daemon.c.

tmux actually already distributes an appropriate replacement version of daemon(3) in its compat/daemon.c, but it is only currently used on systems that lack daemon(3). Furthermore, avoiding the problematic bit of libSystem’s daemon(3) is (unfortunately) only a partial solution.

The README.md section titled The Diagnostic Program describes one issue in the part that starts with “Demonstrate revocation of access to the per-user bootstrap namespace”: using a customized daemon(3) that omits the bootstrap namespace modification (specified by the daemon=ours bit) does let the resulting process continue to access the user’s normal bootstrap namespace, but it is a fragile situation that can be destroyed under certain sequence of events. Specifically, the process will lose access to the user’s normal bootstrap namespace when its originating login session ends.

The example demonstrates this with an SSH connection to the local machine that ends after the call to the custom daemon(3) function, but before the attempt to use pbpaste. That example situation is contrived, but it is easy to test in an automated way.

A similar situation—one that is much more relevant (but not as conveniently testable)—is where the user starts a tmux server under a GUI session and leaves it running while logging out of that GUI session (arguably, one of the prime uses of screen or tmux). In this case, the tmux server (and its children) will lose access to the user’s bootstrap namespace when the GUI session ends; most importantly, access will not be restored when a new GUI session is started.

In short, just swapping out the system deamon(3) with one that does not alter the bootstrap namespace will result in the user only having pasteboard access while running under a tmux server that was started from the current GUI session.

Interestingly, I am unable to reproduce this fragility under 10.8. I do not have a 10.7 machine to test, but 10.6 does (still) show this fragility. I poked around a bit in the 10.6 launchd and xnu sources, but I did not find where the “revocation” is happening. It is difficult to guess whether this change in behavior was intended by Apple, or just a side effect of some other change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants