| Summary: | Fix PTY handling on Mac OS X | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Portable OpenSSH | Reporter: | Disco Vince Giffin <vgiffin> | ||||||||||||
| Component: | ssh | Assignee: | Assigned to nobody <unassigned-bugs> | ||||||||||||
| Status: | CLOSED FIXED | ||||||||||||||
| Severity: | normal | CC: | djm, dtucker | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 4.7p1 | ||||||||||||||
| Hardware: | Other | ||||||||||||||
| OS: | Mac OS X | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 1481 | ||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 1416 [details]
A PTY permission fix.
Could you please explain why these are needed? Does ttyname not work on OS X? And why do you want to not change the ownership of the ptys? (In reply to comment #2) > Could you please explain why these are needed? Does ttyname not work > on OS X? And why do you want to not change the ownership of the ptys? I've spoken with the author of these patches and done some testing. The first patch (for ttyname) is no longer needed. The second (permission fix) patch bypasses setowner. We don't need to call setowner because the ownership and permissions are already setup when we allocate a cloning pty. This patch was added (and is needed) because when thing close down, and the last file descriptor to the pty is closed, the kernel removes it from /dev. So when sshd (thinking the pty is still there) tries to reset the ownership and permissions, it gets an error because it isn't there anymore. (In reply to comment #3) > I've spoken with the author of these patches and done some testing. > > The first patch (for ttyname) is no longer needed. OK then I'll ignore that one. > The second (permission fix) patch bypasses setowner. We don't need to > call setowner because the ownership and permissions are already setup > when we allocate a cloning pty. That means pty_setowner is a no-op? So is the ifdef even required? There's another call to pty_setowner in monitor.c which will be called when privsep is enabled (the default) so the ifdef around pty_setowner will only have an effect if privsep is off or it's root logging in. > This patch was added (and is needed) > because when thing close down, and the last file descriptor to the pty > is closed, the kernel removes it from /dev. So when sshd (thinking the > pty is still there) tries to reset the ownership and permissions, it > gets an error because it isn't there anymore. OK that sounds reasonable but it probably belongs in sshpty.c with the rest of the pty handling code. Created attachment 1439 [details]
Make pty_release a no-op on OS X
If my assumptions are correct, this patch should do the right thing on OS X.
Where/how does __APPLE_PRIVPTY__ set set? (In reply to comment #5) > Created an attachment (id=1439) [details] > Make pty_release a no-op on OS X > > If my assumptions are correct, this patch should do the right thing on > OS X. This patch does work for Mac OS X (10.5.1). (In reply to comment #6) > Where/how does __APPLE_PRIVPTY__ set set? This is set (or not) with an Extra_Configure_Flags in our top level Makefile, depending on what platform we are building for. (In reply to comment #8) > (In reply to comment #6) > > Where/how does __APPLE_PRIVPTY__ set set? > > This is set (or not) with an Extra_Configure_Flags in our top level > Makefile, depending on what platform we are building for. So that means that people building OpenSSH from source themselves on those platforms will not have the #define and thus get the wrong behaviour? How can one reliably tell which platforms/versions need that need this? (In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #6) > > > Where/how does __APPLE_PRIVPTY__ set set? > > > > This is set (or not) with an Extra_Configure_Flags in our top level > > Makefile, depending on what platform we are building for. > > So that means that people building OpenSSH from source themselves on > those platforms will not have the #define and thus get the wrong > behaviour? > > How can one reliably tell which platforms/versions need that need this? Good question. We could use AvailabilityMacros: #ifdef __APPLE__ #include <AvailabilityMacros.h> #if (MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_5) #define __APPLE_PRIVPTY__ #endif #endif Created attachment 1593 [details]
pty_release => noop with feature test macro
Like this?
Created attachment 1594 [details]
unbroken diff
oops, that last diff was made of fail.
(In reply to comment #12) > Created an attachment (id=1594) [details] > unbroken diff Could someone from Apple confirm that attachment 1594 [details] is correct? I'd like to get this in for openssh-5.2. (In reply to comment #12) > Created an attachment (id=1594) [details] > unbroken diff Could someone from Apple confirm that attachment 1594 [details] is correct? I'd like to get this in for openssh-5.2. (In reply to comment #14) > (In reply to comment #12) > > Created an attachment (id=1594) [details] [details] > > unbroken diff > > Could someone from Apple confirm that attachment 1594 [details] is correct? I'd > like to get this in for openssh-5.2. This works for me (on 10.5.6). patch applied - will be in openssh-5.2. Thanks! Close bugs fixed/reviewed for openssh-5.2 release |
Created attachment 1415 [details] Corrects PTY handling. Attached are two patches for building OpenSSH 4.7p1 on Mac OS X. These patches correct PTY handling on Mac OS X.