Better terminal support in Superuser for interactive sessions #181

Merged
merged 9 commits into from Jan 12, 2014

Conversation

Projects
None yet
7 participants
@tan-ce
Contributor

tan-ce commented Aug 30, 2013

Changed the way the daemon and client handles interactive (ie. standard I/O attached to a termnal emulator) sessions.

Main change is that for interactive sessions, the client opens the pts device and passes the slave device path to the daemon, then the daemon opens and dups the slave device.

The client also puts stdin into raw mode, removing buffering and therefore allowing ncurses apps to function as expected.

Finally, the client also waits for SIGWINCH and forwards the terminal window size every time the signal is received.

The changes are fairly extensive because setting up the slave terminal to be the child's controlling TTY correctly has a slightly different flow from using pipes to wrap the child's standard I/O.

tan-ce added some commits Aug 29, 2013

Part 1 of terminal support fix
Changed the daemon to use pseudo-terminals closer to the way SSH or telnet does. Changes are extensive because the slave device was being opened by the wrong end of the connection, ie. the client is the one which should be opening the master ptmx device.

SIGWINCH is still not being handled. The next commit will attempt to address this.
Added SIGWINCH handling and better cleanup
Added a task to forward SIGWINCH from the client to the daemon via the
correct ioctl. Terminal window sizing now seems to work correctly.

Also added signal handler to catch signals which can cause the client to
terminate, allowing the client to restore stdin's termios. Previously,
killing the su client using (for example) SIGTERM would result in the
terminal being left in an unusable state.
Superuser/jni/su/daemon.c
@@ -383,17 +382,234 @@ int run_daemon() {
return -1;
}
+/**

This comment has been minimized.

@koush

koush Aug 30, 2013

Owner

I'm guessing all this comes pasted from another project? glibc maybe? What is the license? Can we put the pts related boilerplate stuff in a separate file? pts.c?

@koush

koush Aug 30, 2013

Owner

I'm guessing all this comes pasted from another project? glibc maybe? What is the license? Can we put the pts related boilerplate stuff in a separate file? pts.c?

This comment has been minimized.

@tan-ce

tan-ce Aug 30, 2013

Contributor

This comes from one of my own projects, Apache 2.0 licensed. It was experiments I did for this exact issue: https://github.com/tan-ce/pts-multi/blob/master/jni/helpers.c#L288

The code was written mostly based off the man pages by myself, not copied wholesale from anywhere. At worst, I might have copied some lines off some example code.

@tan-ce

tan-ce Aug 30, 2013

Contributor

This comes from one of my own projects, Apache 2.0 licensed. It was experiments I did for this exact issue: https://github.com/tan-ce/pts-multi/blob/master/jni/helpers.c#L288

The code was written mostly based off the man pages by myself, not copied wholesale from anywhere. At worst, I might have copied some lines off some example code.

This comment has been minimized.

@koush

koush Aug 30, 2013

Owner

cool, that works. Let's break those into separate files to keep the pts code separate from the daemon code.

@koush

koush Aug 30, 2013

Owner

cool, that works. Let's break those into separate files to keep the pts code separate from the daemon code.

Superuser/jni/su/daemon.c
}
close(input);
- close(output);
+ if (output != STDOUT_FILENO) close(output);

This comment has been minimized.

@koush

koush Aug 30, 2013

Owner

Let's add an overloaded version of pump or an additional argument that specifies whether the output should be closed on finish.

@koush

koush Aug 30, 2013

Owner

Let's add an overloaded version of pump or an additional argument that specifies whether the output should be closed on finish.

This comment has been minimized.

@tan-ce

tan-ce Aug 30, 2013

Contributor

Ok, sounds like a good idea.

@tan-ce

tan-ce Aug 30, 2013

Contributor

Ok, sounds like a good idea.

Superuser/jni/su/daemon.c
@@ -106,21 +109,31 @@ static int run_daemon_child(int infd, int outfd, int errfd, int argc, char** arg
exit(-1);
}
- close(infd);

This comment has been minimized.

@koush

koush Aug 30, 2013

Owner

Why were these removed? This leaves dangling file descriptors. The only file descriptors needed beyond this point are STD*

@koush

koush Aug 30, 2013

Owner

Why were these removed? This leaves dangling file descriptors. The only file descriptors needed beyond this point are STD*

This comment has been minimized.

@tan-ce

tan-ce Aug 30, 2013

Contributor

From the Linux man page for dup2, those descriptors are automatically closed. If I'm not mistaken, then only case where they are not automatically closed is where newfd == oldfd.

@tan-ce

tan-ce Aug 30, 2013

Contributor

From the Linux man page for dup2, those descriptors are automatically closed. If I'm not mistaken, then only case where they are not automatically closed is where newfd == oldfd.

This comment has been minimized.

@koush

koush Aug 30, 2013

Owner

You misunderstand the man page.

int dup2(int oldfd, int newfd);

dup2() makes newfd be the copy of oldfd, closing newfd first if necessary

That means newfd (STD*) is closed. The infd (the fifos), etc, are not closed. Those need to be closed, as they were duped.

@koush

koush Aug 30, 2013

Owner

You misunderstand the man page.

int dup2(int oldfd, int newfd);

dup2() makes newfd be the copy of oldfd, closing newfd first if necessary

That means newfd (STD*) is closed. The infd (the fifos), etc, are not closed. Those need to be closed, as they were duped.

This comment has been minimized.

@tan-ce

tan-ce Aug 30, 2013

Contributor

Oh sorry, my bad, I'll revert the change.

@tan-ce

tan-ce Aug 30, 2013

Contributor

Oh sorry, my bad, I'll revert the change.

Superuser/jni/su/daemon.c
- if (ptm <= 0) {
- PLOGE("ptm");
+ // Get the PTS slave device name
+ pts_slave = read_string(fd);

This comment has been minimized.

@koush

koush Aug 30, 2013

Owner

Let's always send a pts_slave value across. I don't like branching the logic and having a inconsistent wire protocol. If its not used, send an empty string.

Alternatively, get rid of the atty flag completely, and simply send a string across. non zero length string of pts_slave implies an atty.

@koush

koush Aug 30, 2013

Owner

Let's always send a pts_slave value across. I don't like branching the logic and having a inconsistent wire protocol. If its not used, send an empty string.

Alternatively, get rid of the atty flag completely, and simply send a string across. non zero length string of pts_slave implies an atty.

This comment has been minimized.

@koush

koush Aug 30, 2013

Owner

Ie:

char* pts = read_string(...)
int atty = strlen(pts) != 0;
@koush

koush Aug 30, 2013

Owner

Ie:

char* pts = read_string(...)
int atty = strlen(pts) != 0;

This comment has been minimized.

@tan-ce

tan-ce Aug 30, 2013

Contributor

Ok, the part on the protocol sounds good, and I can also save on the atty flag.

Actually, on this note, I am not too happy with all the branching logic either. What do you think about factoring out the common logic (eg. permission check) and having two different functions, one for TTYs.

But we might want to put some further thought into this first. Right now the behaviour of su in pipes is not completely correct. For example,

echo "foo bar" | su -c /system/bin/less

doesn't behave as expected, because su's stdout is a TTY, so it asks for a pseudo-terminal from the daemon. However, that means less' stdin is now also a terminal, so it adopts fully interactive behaviour, and doesn't accept input to display on stdin. However, if we change the atty check to look at stdin, then less will not be able to display the output correctly, because its output is now attached to FIFOs.

I think we need to look at each I/O stream (stdin, stdout, stderr) and process them separately, establishing a pty in the daemon where the su client's I/O stream is attached to a tty, or a FIFO otherwise. But this also sounds like a large change, and I'm inclined to leave it for a later commit, and just get the basics right first.

@tan-ce

tan-ce Aug 30, 2013

Contributor

Ok, the part on the protocol sounds good, and I can also save on the atty flag.

Actually, on this note, I am not too happy with all the branching logic either. What do you think about factoring out the common logic (eg. permission check) and having two different functions, one for TTYs.

But we might want to put some further thought into this first. Right now the behaviour of su in pipes is not completely correct. For example,

echo "foo bar" | su -c /system/bin/less

doesn't behave as expected, because su's stdout is a TTY, so it asks for a pseudo-terminal from the daemon. However, that means less' stdin is now also a terminal, so it adopts fully interactive behaviour, and doesn't accept input to display on stdin. However, if we change the atty check to look at stdin, then less will not be able to display the output correctly, because its output is now attached to FIFOs.

I think we need to look at each I/O stream (stdin, stdout, stderr) and process them separately, establishing a pty in the daemon where the su client's I/O stream is attached to a tty, or a FIFO otherwise. But this also sounds like a large change, and I'm inclined to leave it for a later commit, and just get the basics right first.

This comment has been minimized.

@koush

koush Aug 30, 2013

Owner

Agreed, let's tackle that in the context of another change.

@koush

koush Aug 30, 2013

Owner

Agreed, let's tackle that in the context of another change.

@koush

This comment has been minimized.

Show comment
Hide comment
@koush

koush Aug 30, 2013

Owner

Thanks for the effort thus far. Looking good, only a few issues.

Let's address my initial comments, and I'll take another pass at it

Owner

koush commented Aug 30, 2013

Thanks for the effort thus far. Looking good, only a few issues.

Let's address my initial comments, and I'll take another pass at it

@tan-ce

This comment has been minimized.

Show comment
Hide comment
@tan-ce

tan-ce Aug 30, 2013

Contributor

Thanks for taking the time to review the code.

I've made some more changes which I think should address your initial comments. Do let me know your comments.

Contributor

tan-ce commented Aug 30, 2013

Thanks for taking the time to review the code.

I've made some more changes which I think should address your initial comments. Do let me know your comments.

Superuser/jni/su/daemon.c
- chmod(infile, 0660);
- chmod(errfile, 0660);
+ // Get the PTS slave device name
+ pts_slave = read_string(fd);

This comment has been minimized.

@koush

koush Aug 31, 2013

Owner

Let's put this at the top, with the other read_strings, before the args are read. I prefer the variable length args bit at the end.
Couldn't this also replace atty=read_int() as discussed?

@koush

koush Aug 31, 2013

Owner

Let's put this at the top, with the other read_strings, before the args are read. I prefer the variable length args bit at the end.
Couldn't this also replace atty=read_int() as discussed?

This comment has been minimized.

@tan-ce

tan-ce Sep 1, 2013

Contributor

Ok, will relocate the slave name to before the args.

For atty, I left it in there because I'm thinking of eventually turning it into a bitfield indicating whether each of the I/O streams are FIFOs or PTY bound. This, I think should give us the correct behaviour when su is used in pipes.

@tan-ce

tan-ce Sep 1, 2013

Contributor

Ok, will relocate the slave name to before the args.

For atty, I left it in there because I'm thinking of eventually turning it into a bitfield indicating whether each of the I/O streams are FIFOs or PTY bound. This, I think should give us the correct behaviour when su is used in pipes.

@koush

This comment has been minimized.

Show comment
Hide comment
@koush

koush Aug 31, 2013

Owner

This is looking pretty good, only had a question regarding atty and pts_name essentially being redundant (which also needs to be moved up to the top)

Owner

koush commented Aug 31, 2013

This is looking pretty good, only had a question regarding atty and pts_name essentially being redundant (which also needs to be moved up to the top)

Superuser/jni/su/daemon.c
+ sprintf(outfile, "%s/%d.stdout", REQUESTOR_DAEMON_PATH, getpid());
+ sprintf(errfile, "%s/%d.stderr", REQUESTOR_DAEMON_PATH, getpid());
+ sprintf(infile, "%s/%d.stdin", REQUESTOR_DAEMON_PATH, getpid());
+ unlink(errfile);

This comment has been minimized.

@tan-ce

tan-ce Sep 1, 2013

Contributor

@koush, this unlink is called from the client, correct? Does the client have the permissions to unlink these files? Shouldn't this unlink be called by the daemon just before the call to mkfifo?

@tan-ce

tan-ce Sep 1, 2013

Contributor

@koush, this unlink is called from the client, correct? Does the client have the permissions to unlink these files? Shouldn't this unlink be called by the daemon just before the call to mkfifo?

@mkasick

This comment has been minimized.

Show comment
Hide comment
@mkasick

mkasick Sep 1, 2013

Contributor

Instead of going to all the trouble to create a new PTY and pump file data between the master and slave processes, why not just send the file descriptors themselves across the socket?

This way the slave process reuses the existing PTY (if there is one), and it avoids the syscall overhead of pumping file data around. It's essentially the same behavior as non-daemonized su.

Is there a reason to avoid that approach that I'm missing?

Contributor

mkasick commented Sep 1, 2013

Instead of going to all the trouble to create a new PTY and pump file data between the master and slave processes, why not just send the file descriptors themselves across the socket?

This way the slave process reuses the existing PTY (if there is one), and it avoids the syscall overhead of pumping file data around. It's essentially the same behavior as non-daemonized su.

Is there a reason to avoid that approach that I'm missing?

@koush

This comment has been minimized.

Show comment
Hide comment
@koush

koush Sep 1, 2013

Owner

@mkasick ie, in the daemon_accept like this?

(pseudocode)

int pid = read_int(fd);
int remote_in = open("/proc/<--insert-pid-->/fd/0", READ);
int remote_out = open("/proc/<--insert-pid-->/fd/1", WRITE);
int remote_err = open("/proc/<--insert-pid-->/fd/2", WRITE);

dup2(STDIN_FILENO, remote_in);
dup2(STDOUT_FILENO, remote_out);
dup2(STDERR_FILENO, remote_err);

Dunno if those file descriptors will be remotely recognizable as a pseudoterminal. It's worth checking.

Owner

koush commented Sep 1, 2013

@mkasick ie, in the daemon_accept like this?

(pseudocode)

int pid = read_int(fd);
int remote_in = open("/proc/<--insert-pid-->/fd/0", READ);
int remote_out = open("/proc/<--insert-pid-->/fd/1", WRITE);
int remote_err = open("/proc/<--insert-pid-->/fd/2", WRITE);

dup2(STDIN_FILENO, remote_in);
dup2(STDOUT_FILENO, remote_out);
dup2(STDERR_FILENO, remote_err);

Dunno if those file descriptors will be remotely recognizable as a pseudoterminal. It's worth checking.

@mkasick

This comment has been minimized.

Show comment
Hide comment
@mkasick

mkasick Sep 2, 2013

Contributor

@koush: No, actually, there's some socket voodoo that actually lets you transfer an open file descriptor, including permissions, offsets, etc., to a different process. system/core/libcutils/mq.c:470:peerProxyWriteConnection contains an implementation of this for sending the fd of an open socket, though you can use it for any file descriptor. mq.c:719:masterProxyAcceptConnection contains the code for the other side in the receiving process. There's other packages in the Android tree that implement essentially the same routine (grep SCM_RIGHTS).

I have a test implementation of this working, the problem I'm trying to figure out right now is how to properly transfer the controlling tty of the original pty. It's easy enough to steal, but su needs to "give it back" to the calling shell when it's finished. It might be that opening another pty is the only clean solution here, but transferring the file descriptors would definitely eliminate the need for the extra FIFOs.

Contributor

mkasick commented Sep 2, 2013

@koush: No, actually, there's some socket voodoo that actually lets you transfer an open file descriptor, including permissions, offsets, etc., to a different process. system/core/libcutils/mq.c:470:peerProxyWriteConnection contains an implementation of this for sending the fd of an open socket, though you can use it for any file descriptor. mq.c:719:masterProxyAcceptConnection contains the code for the other side in the receiving process. There's other packages in the Android tree that implement essentially the same routine (grep SCM_RIGHTS).

I have a test implementation of this working, the problem I'm trying to figure out right now is how to properly transfer the controlling tty of the original pty. It's easy enough to steal, but su needs to "give it back" to the calling shell when it's finished. It might be that opening another pty is the only clean solution here, but transferring the file descriptors would definitely eliminate the need for the extra FIFOs.

@tan-ce

This comment has been minimized.

Show comment
Hide comment
@tan-ce

tan-ce Sep 2, 2013

Contributor

Some sequences (eg. C-c, C-z) and at least one event (SIGWINCH) are handled by the controlling TTY driver and interacts only to the foreground process of the original terminal. Because of this, I'm inclined to keep the pty.

But still, @mkasick, what did you mean by "transfer the controlling tty of the original pty"?

But I did some experiments on the command line, and it does look like we could at least use /proc/%d/fd in place of the FIFOs.

Contributor

tan-ce commented Sep 2, 2013

Some sequences (eg. C-c, C-z) and at least one event (SIGWINCH) are handled by the controlling TTY driver and interacts only to the foreground process of the original terminal. Because of this, I'm inclined to keep the pty.

But still, @mkasick, what did you mean by "transfer the controlling tty of the original pty"?

But I did some experiments on the command line, and it does look like we could at least use /proc/%d/fd in place of the FIFOs.

@mkasick

This comment has been minimized.

Show comment
Hide comment
@mkasick

mkasick Sep 2, 2013

Contributor

@tan-ce So if you send the fd of the original terminal/pty to the child process, calling "ioctl(orig_tty, TIOCSCTTY, 1)" steals the terminal from the old process session and assigns it as the controlling terminal of the new (from setsid) session. Doing so allows reuse of the original terminal in the new session with working job control.

The problem is that when the new session exits (leave the su shell), the old session no longer has a controlling terminal so job control is broken. I can't find a way to "assign back" the controlling terminal to the old session without having control of the sesion leader. This may be why using a second pty is necessary.

Contributor

mkasick commented Sep 2, 2013

@tan-ce So if you send the fd of the original terminal/pty to the child process, calling "ioctl(orig_tty, TIOCSCTTY, 1)" steals the terminal from the old process session and assigns it as the controlling terminal of the new (from setsid) session. Doing so allows reuse of the original terminal in the new session with working job control.

The problem is that when the new session exits (leave the su shell), the old session no longer has a controlling terminal so job control is broken. I can't find a way to "assign back" the controlling terminal to the old session without having control of the sesion leader. This may be why using a second pty is necessary.

@tan-ce

This comment has been minimized.

Show comment
Hide comment
@tan-ce

tan-ce Sep 2, 2013

Contributor

Right, I just remembered that there was a Linux app called retty which does something like this. Still, we still need to think about cases where a TTY is not gong to be attached to the standard I/O, but I'm more inclined to use /proc/%d/fd because it would involve much less code.

Anyway, I think what I'll do for now is to do TTY detection on each I/O stream and replace the FIFOs in /dev/ with opening the process' file descriptors directly

Contributor

tan-ce commented Sep 2, 2013

Right, I just remembered that there was a Linux app called retty which does something like this. Still, we still need to think about cases where a TTY is not gong to be attached to the standard I/O, but I'm more inclined to use /proc/%d/fd because it would involve much less code.

Anyway, I think what I'll do for now is to do TTY detection on each I/O stream and replace the FIFOs in /dev/ with opening the process' file descriptors directly

@tan-ce

This comment has been minimized.

Show comment
Hide comment
@tan-ce

tan-ce Sep 2, 2013

Contributor

Did an experiment directly opening the file descriptor of the client. The biggest problem is that EOFs and stream closing doesn't seem to be passed along, so when use in pipes, su never terminates for some commands.

Contributor

tan-ce commented Sep 2, 2013

Did an experiment directly opening the file descriptor of the client. The biggest problem is that EOFs and stream closing doesn't seem to be passed along, so when use in pipes, su never terminates for some commands.

@mkasick

This comment has been minimized.

Show comment
Hide comment
@mkasick

mkasick Sep 2, 2013

Contributor

I made a separate pull request that includes the changes I've been working on to send file descriptors over the socket and reuse the existing terminal. To the extent I've tested it, it works decently well caveat that an unmodified caller shell will lose job control functionality after su is complete.

I think adding a proper pty proxy on top of that would alleviate the job control issue, but I'm not sure what else it would buy (we're not doing multiplexing or remoting). If one wants to do that, probably the best approach is to open the pty in the "remote" process, send_fd the slave descriptor to the daemon, and use sendfile(2) to cheaply shuttle data between the pty master and the remote's terminal.

Contributor

mkasick commented Sep 2, 2013

I made a separate pull request that includes the changes I've been working on to send file descriptors over the socket and reuse the existing terminal. To the extent I've tested it, it works decently well caveat that an unmodified caller shell will lose job control functionality after su is complete.

I think adding a proper pty proxy on top of that would alleviate the job control issue, but I'm not sure what else it would buy (we're not doing multiplexing or remoting). If one wants to do that, probably the best approach is to open the pty in the "remote" process, send_fd the slave descriptor to the daemon, and use sendfile(2) to cheaply shuttle data between the pty master and the remote's terminal.

@koush

This comment has been minimized.

Show comment
Hide comment
@koush

koush Sep 5, 2013

Owner

There seem to be two issues here, that we may be jumbling into one change:

  • send_fd vs fifos
  • pseudoterminals

Pseudoterminals aside, is there any downside to using send_fd for the regular non-terminal path? That seems like it would greatly simplify the piping code, as @mkasick mentioned.

Owner

koush commented Sep 5, 2013

There seem to be two issues here, that we may be jumbling into one change:

  • send_fd vs fifos
  • pseudoterminals

Pseudoterminals aside, is there any downside to using send_fd for the regular non-terminal path? That seems like it would greatly simplify the piping code, as @mkasick mentioned.

@tan-ce

This comment has been minimized.

Show comment
Hide comment
@tan-ce

tan-ce Sep 6, 2013

Contributor

Hmm. For the regular non-terminal path, I attempted to open the FDs directly via the /proc filesystem, but that resulted in su not terminating in pipes, I believe because the FDs were not closing properly.

Perhaps I should merge some of @mkasick's code and see if closing works correctly when using send_fd for the fully non-terminal path. I'll see if I can try this out this weekend and see how it works out.

Contributor

tan-ce commented Sep 6, 2013

Hmm. For the regular non-terminal path, I attempted to open the FDs directly via the /proc filesystem, but that resulted in su not terminating in pipes, I believe because the FDs were not closing properly.

Perhaps I should merge some of @mkasick's code and see if closing works correctly when using send_fd for the fully non-terminal path. I'll see if I can try this out this weekend and see how it works out.

@mkasick

This comment has been minimized.

Show comment
Hide comment
@mkasick

mkasick Sep 6, 2013

Contributor

I'm not sure that opening files via /proc is equivalent to send_fd. With send_fd, the recipient process receives a clone of the kernel-level file descriptor including permissions, offset, etc. When a process opens the file using /proc, I believe it just follows the symlink and otherwise opens it with an independent descriptor. So, the process would need the correct permissions, start at offset 0, and (I'd guess) it would fail if the file had been unlinked.

@koush: Yes there's really two issues. A pty proxy can be implemented on top of send_fd, and that's what I'd recommend doing. I can help with that, if we've otherwise decided that send_fd is the route we want to go.

Contributor

mkasick commented Sep 6, 2013

I'm not sure that opening files via /proc is equivalent to send_fd. With send_fd, the recipient process receives a clone of the kernel-level file descriptor including permissions, offset, etc. When a process opens the file using /proc, I believe it just follows the symlink and otherwise opens it with an independent descriptor. So, the process would need the correct permissions, start at offset 0, and (I'd guess) it would fail if the file had been unlinked.

@koush: Yes there's really two issues. A pty proxy can be implemented on top of send_fd, and that's what I'd recommend doing. I can help with that, if we've otherwise decided that send_fd is the route we want to go.

@tan-ce

This comment has been minimized.

Show comment
Hide comment
@tan-ce

tan-ce Sep 6, 2013

Contributor

@mkasick,yup, I realised that now. So I'm going to try to rip out the FIFOs and pumps and merge in the code from your pull request then test its behaviour again.

Contributor

tan-ce commented Sep 6, 2013

@mkasick,yup, I realised that now. So I'm going to try to rip out the FIFOs and pumps and merge in the code from your pull request then test its behaviour again.

@koush

This comment has been minimized.

Show comment
Hide comment
@koush

koush Sep 6, 2013

Owner

Yeah, I think send_fd is the way to go. But if we make that change, I don't want it to be at the cost of a regression in pty usage.

If we need to have a separate code path that handles pty properly/differently (as @tan-ce's code does here), I'm fine with that. But it sounds as if you have some ideas to make ptys work with send_fd.

Owner

koush commented Sep 6, 2013

Yeah, I think send_fd is the way to go. But if we make that change, I don't want it to be at the cost of a regression in pty usage.

If we need to have a separate code path that handles pty properly/differently (as @tan-ce's code does here), I'm fine with that. But it sounds as if you have some ideas to make ptys work with send_fd.

Eliminated use of FIFOs, factored PTY proxy code
Instead of opening FIFOs, we now send the standard I/O descriptors
across the Unix socket.

Also, the I/O pumps were moved to pts.c
@tan-ce

This comment has been minimized.

Show comment
Hide comment
@tan-ce

tan-ce Sep 8, 2013

Contributor

Okay, integrated @mkasick's send/recv_fd and eliminated the FIFOs. I realised I still need the pumps for the PTY proxy, so I moved them to pts.c.

What system property should we use to disable PTY proxying?

Contributor

tan-ce commented Sep 8, 2013

Okay, integrated @mkasick's send/recv_fd and eliminated the FIFOs. I realised I still need the pumps for the PTY proxy, so I moved them to pts.c.

What system property should we use to disable PTY proxying?

@koush

This comment has been minimized.

Show comment
Hide comment
@koush

koush Nov 3, 2013

Owner

Sorry, lost track of this. Can I submit? What was the need for a system property to disable pty proxying?

Owner

koush commented Nov 3, 2013

Sorry, lost track of this. Can I submit? What was the need for a system property to disable pty proxying?

@tan-ce

This comment has been minimized.

Show comment
Hide comment
@tan-ce

tan-ce Nov 4, 2013

Contributor

No problem, I figured the CyanogenMod announcement was keeping you busy. =)

mkasick was suggesting a system property for disabling the spawning of another pty for those who do not wish to use that on their systems. In that case, setting that system property to true will cause su to only use the FD transfer mechanism.

In any case, feel free to pull/submit the code whenever you feel it's ready.

Contributor

tan-ce commented Nov 4, 2013

No problem, I figured the CyanogenMod announcement was keeping you busy. =)

mkasick was suggesting a system property for disabling the spawning of another pty for those who do not wish to use that on their systems. In that case, setting that system property to true will cause su to only use the FD transfer mechanism.

In any case, feel free to pull/submit the code whenever you feel it's ready.

@tan-ce

This comment has been minimized.

Show comment
Hide comment
@tan-ce

tan-ce Dec 26, 2013

Contributor

Hi @koush, just checking, any update on this pull request?

Contributor

tan-ce commented Dec 26, 2013

Hi @koush, just checking, any update on this pull request?

@koush

This comment has been minimized.

Show comment
Hide comment
@koush

koush Jan 12, 2014

Owner

Just tried merging this and testing. Command line su invocation works, but invocation from app does not.

D/su ( 4395): su invoked.
D/su ( 4395): starting daemon client 10107 10107
D/su ( 4398): remote pid: 4395
D/su ( 4398): remote pts_slave:
E/su ( 4398): unable to read int
E/su ( 4395): unable to read int

Owner

koush commented Jan 12, 2014

Just tried merging this and testing. Command line su invocation works, but invocation from app does not.

D/su ( 4395): su invoked.
D/su ( 4395): starting daemon client 10107 10107
D/su ( 4398): remote pid: 4395
D/su ( 4398): remote pts_slave:
E/su ( 4398): unable to read int
E/su ( 4395): unable to read int

@koush koush merged commit 8881f70 into koush:master Jan 12, 2014

@15607007919

This comment has been minimized.

Show comment
Hide comment
@15607007919

15607007919 May 20, 2014

Something's wrong with the way that Superuser (or maybe the su binary) interacts with

adb bugreport

If I do

adb shell /system/xbin/su something

then superuser access is (correctly) granted quickly and silently.

Running adb bugreport, however, causes a crapload of toast messages. If I Ctrl-C adb bugreport quickly enough, the toasts sit there for a long time. If I let adb bugreport run long enough, su starts timing out and the phone becomes unusable (and seems to never recover).

Something's wrong with the way that Superuser (or maybe the su binary) interacts with

adb bugreport

If I do

adb shell /system/xbin/su something

then superuser access is (correctly) granted quickly and silently.

Running adb bugreport, however, causes a crapload of toast messages. If I Ctrl-C adb bugreport quickly enough, the toasts sit there for a long time. If I let adb bugreport run long enough, su starts timing out and the phone becomes unusable (and seems to never recover).

@mkasick

This comment has been minimized.

Show comment
Hide comment
@mkasick

mkasick May 20, 2014

Contributor

Hi,

In the future please create a new issue, or update an appropriate existing issue, instead of commenting on a closed pull request.

The issue with bugreport is that dumpstate opportunistically calls /system/xbin/su many, many times, specifically to run showmap on every running process. This works OK on AOSP eng builds where the simpler version of su (system/extras) does not take long to execute, but is pretty much incompatible with a more sophisticated su.

In CyanogenMod, we've changed dumpstate [1] to avoid calling showmap an excessive number of times, which makes it much faster to generate bugreports. For AOSP ROMs, I'd recommend installing Superuser only as /system/bin/su, not /system/xbin/su, so that dumpstate no longer calls showmap.

[1] CyanogenMod/android_frameworks_native@1ba6870

Contributor

mkasick commented May 20, 2014

Hi,

In the future please create a new issue, or update an appropriate existing issue, instead of commenting on a closed pull request.

The issue with bugreport is that dumpstate opportunistically calls /system/xbin/su many, many times, specifically to run showmap on every running process. This works OK on AOSP eng builds where the simpler version of su (system/extras) does not take long to execute, but is pretty much incompatible with a more sophisticated su.

In CyanogenMod, we've changed dumpstate [1] to avoid calling showmap an excessive number of times, which makes it much faster to generate bugreports. For AOSP ROMs, I'd recommend installing Superuser only as /system/bin/su, not /system/xbin/su, so that dumpstate no longer calls showmap.

[1] CyanogenMod/android_frameworks_native@1ba6870

@koush

This comment has been minimized.

Show comment
Hide comment
@koush

koush May 20, 2014

Owner

I swear this bugreport issue was fixed in superuser. Let me look.

Owner

koush commented May 20, 2014

I swear this bugreport issue was fixed in superuser. Let me look.

@koush

This comment has been minimized.

Show comment
Hide comment
@koush

koush May 20, 2014

Owner

Yeah ->
f465a0a

Owner

koush commented May 20, 2014

Yeah ->
f465a0a

@jaickeyjack007

This comment has been minimized.

Show comment
Hide comment
@jaickeyjack007

jaickeyjack007 Feb 4, 2018

Jow to use that

Jow to use that

This comment has been minimized.

Show comment
Hide comment

Gjko

@zaaraameera2014

This comment has been minimized.

Show comment
Hide comment

gghjkk

@igor074

This comment has been minimized.

Show comment
Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment