Merge "adb: fix subprocess termination for legacy shell." am: a9352202dd (7fd384bf) · Commits · e / os / android_system_core

adb/commandline.cpp

+11 −4

Original line number	Diff line number	Diff line
		@@ -768,10 +768,6 @@ static int adb_shell(int argc, const char** argv,
		argc -= 2;
		argv += 2;
		} else if (!strcmp(argv[0], "-T") \|\| !strcmp(argv[0], "-t")) {
		if (!CanUseFeature(features, kFeatureShell2)) {
		fprintf(stderr, "error: target doesn't support PTY args -Tt\n");
		return 1;
		}
		// Like ssh, -t arguments are cumulative so that multiple -t's
		// are needed to force a PTY.
		if (argv[0][1] == 't') {
		@@ -795,6 +791,17 @@ static int adb_shell(int argc, const char** argv,
		}
		}

		// Legacy shell protocol requires a remote PTY to close the subprocess properly which creates
		// some weird interactions with -tT.
		if (!use_shell_protocol && t_arg_count != 0) {
		if (!CanUseFeature(features, kFeatureShell2)) {
		fprintf(stderr, "error: target doesn't support PTY args -Tt\n");
		} else {
		fprintf(stderr, "error: PTY args -Tt cannot be used with -x\n");
		}
		return 1;
		}

		std::string shell_type_arg;
		if (CanUseFeature(features, kFeatureShell2)) {
		if (t_arg_count < 0) {

adb/shell_service.cpp

+31 −0

Original line number	Diff line number	Diff line
		@@ -212,6 +212,7 @@ class Subprocess {

		const std::string command_;
		const std::string terminal_type_;
		bool make_pty_raw_ = false;
		SubprocessType type_;
		SubprocessProtocol protocol_;
		pid_t pid_ = -1;
		@@ -231,6 +232,18 @@ Subprocess::Subprocess(const std::string& command, const char* terminal_type,
		terminal_type_(terminal_type ? terminal_type : ""),
		type_(type),
		protocol_(protocol) {
		// If we aren't using the shell protocol we must allocate a PTY to properly close the
		// subprocess. PTYs automatically send SIGHUP to the slave-side process when the master side
		// of the PTY closes, which we rely on. If we use a raw pipe, processes that don't read/write,
		// e.g. screenrecord, will never notice the broken pipe and terminate.
		// The shell protocol doesn't require a PTY because it's always monitoring the local socket FD
		// with select() and will send SIGHUP manually to the child process.
		if (protocol_ == SubprocessProtocol::kNone && type_ == SubprocessType::kRaw) {
		// Disable PTY input/output processing since the client is expecting raw data.
		D("Can't create raw subprocess without shell protocol, using PTY in raw mode instead");
		type_ = SubprocessType::kPty;
		make_pty_raw_ = true;
		}
		}

		Subprocess::~Subprocess() {
		@@ -416,6 +429,24 @@ int Subprocess::OpenPtyChildFd(const char* pts_name, ScopedFd* error_sfd) {
		exit(-1);
		}

		if (make_pty_raw_) {
		termios tattr;
		if (tcgetattr(child_fd, &tattr) == -1) {
		int saved_errno = errno;
		WriteFdExactly(error_sfd->fd(), "tcgetattr failed: ");
		WriteFdExactly(error_sfd->fd(), strerror(saved_errno));
		exit(-1);
		}

		cfmakeraw(&tattr);
		if (tcsetattr(child_fd, TCSADRAIN, &tattr) == -1) {
		int saved_errno = errno;
		WriteFdExactly(error_sfd->fd(), "tcsetattr failed: ");
		WriteFdExactly(error_sfd->fd(), strerror(saved_errno));
		exit(-1);
		}
		}

		return child_fd;
		}

adb/shell_service_test.cpp

+3 −2

Original line number	Diff line number	Diff line
		@@ -175,8 +175,9 @@ TEST_F(ShellServiceTest, RawNoProtocolSubprocess) {
		"echo foo; echo bar >&2; [ -t 0 ]; echo $?",
		SubprocessType::kRaw, SubprocessProtocol::kNone));

		// [ -t 0 ] == 1 means no terminal (raw).
		ExpectLinesEqual(ReadRaw(subprocess_fd_), {"foo", "bar", "1"});
		// [ -t 0 ] == 0 means we have a terminal (PTY). Even when requesting a raw subprocess, without
		// the shell protocol we should always force a PTY to ensure proper cleanup.
		ExpectLinesEqual(ReadRaw(subprocess_fd_), {"foo", "bar", "0"});
		}

		// Tests a PTY subprocess with no protocol.