From a25602788c1b5788adddbbb51aa0d53a15b725df Mon Sep 17 00:00:00 2001 From: Daniel Colascione Date: Mon, 17 Oct 2016 13:51:55 -0700 Subject: [PATCH] Always get a PTY even with newer adb New versions of ADB have improved the shell command so that it's clean, propagates error codes, and allocates PTYs unconditionally, much like ssh. This last change breaks fb-adb, since we run the inferior adb with pipes for stdio; ADB thinks it's being run non-interactively, skips allocating a PTY on the device side. The remote shell doesn't have a pty itself, so _it_ thinks it's being run non-interactively, so it doesn't print a prompt. Meanwhile, fb-adb hangs because it's waiting forever for a prompt. This change just gets us back to using a PTY unconditionally. We could take advantage of adb fixes to get rid of some of our encoding logic, but that's a separate project. --- chat.c | 14 +++++- cmd_shex.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++------ util.c | 29 +++++++++++++ util.h | 12 ++++++ 4 files changed, 162 insertions(+), 15 deletions(-) diff --git a/chat.c b/chat.c index b28d881..0ee6787 100644 --- a/chat.c +++ b/chat.c @@ -70,12 +70,22 @@ chat_expect_maybe(struct chat* cc, char expected) static void chat_swallow_prompt(struct chat* cc) { + SCOPED_RESLIST(rl); + /* 100% reliable prompt detection */ unsigned csi_arg = 0; enum { S_NORMAL, S_AFTER_ESC, S_AFTER_CSI } state = S_NORMAL; + struct growable_string pre_prompt = {}; for (;;) { - char c = chat_getc(cc); + int char_read = getc(cc->from); + if (char_read == EOF) { + growable_string_trim_trailing_whitespace(&pre_prompt); + if (pre_prompt.strlen == 0) + chat_die(); + die(ECOMM, "%s", growable_string_c_str(&pre_prompt)); + } + char c = char_read; if (c == '#' || c == '$') break; @@ -93,6 +103,8 @@ chat_swallow_prompt(struct chat* cc) case S_NORMAL: if (c == '\033') state = S_AFTER_ESC; + else + growable_string_append_c(&pre_prompt, c); break; case S_AFTER_ESC: diff --git a/cmd_shex.c b/cmd_shex.c index 7476ba4..5bd96e4 100644 --- a/cmd_shex.c +++ b/cmd_shex.c @@ -232,10 +232,10 @@ send_stub(const void* data, static struct child* -try_adb_stub(const struct child_start_info* csi, - const char* adb_name, - struct child_hello* chello, - char** err) +try_adb_stub_2(const struct child_start_info* csi, + const char* adb_name, + struct child_hello* chello, + char** err) { SCOPED_RESLIST(rl); struct child* child = child_start(csi); @@ -301,6 +301,104 @@ try_adb_stub(const struct child_start_info* csi, return NULL; } +struct try_adb_stub_internal_info { + struct child* ret; + const struct child_start_info* csi; + const char* adb_name; + struct child_hello* chello; + char** err; +}; + +static void +try_adb_stub_1(void* arg) +{ + struct try_adb_stub_internal_info* info = arg; + info->ret = try_adb_stub_2( + info->csi, info->adb_name, info->chello, info->err); +} + +struct try_adb_stub_memory { + bool old_adb_detected; +}; + +static struct child* +try_adb_stub(struct try_adb_stub_memory* tasm, + const char* const* adb_args, + const char* adb_name, + struct child_hello* chello, + char** err) +{ + const struct child_start_info csi = { + .io[STDIN_FILENO] = CHILD_IO_PIPE, + .io[STDOUT_FILENO] = CHILD_IO_PIPE, + .io[STDERR_FILENO] = CHILD_IO_RECORD, + .exename = "adb", + .argv = ARGV_CONCAT(ARGV("adb"), + adb_args, + tasm->old_adb_detected + ? ARGV("shell") + : ARGV("shell", "-t", "-t")), + }; + + struct try_adb_stub_internal_info info = { + .csi = &csi, + .adb_name = adb_name, + .chello = chello, + .err = err, + }; + + struct errinfo ei = { .want_msg = true }; + if (catch_error(try_adb_stub_1, &info, &ei)) { + if (ei.err == ECOMM && + ei.msg && + (string_ends_with_p( + ei.msg, "-t: unknown option" /* E1 */) || + string_ends_with_p( + ei.msg, + "target doesn't support PTY args -Tt" /* E2 */ ))) + { + /* + + Here's what happens under various combinations of tool and + device versions when stdin is a pipe, not a tty. + + Why doesn't "-t -t" just succeed when running a new ADB + against an older device, where we'll get a tty anyway? + Because the universe is malevolent. See the following + outcome table. E1 and E2 refer to the messages + just above. + + Pre-N device | Post-N device + + --------------+---------------- + Pre-N ADB | E1 | E1 + -t -t | | + -----------+---------------+---------------- + Pre-N ADB | Get a TTY | Get a TTY + no options | | + -----------+---------------+---------------- + Post-N ADB | E2 | Get a TTY + -t -t | | + -----------+---------------+---------------- + Post-N ADB | Get a TTY | Get a pipe (and hang) + no options | + + If we react to _either_ E1 or E2 by re-invoking without -t + -t, we get a TTY. The downside of this approach is that + we need a second "adb shell" invocation when we have a new + adb connecting to an old device, but hopefully, users will + just rely on the daemon and avoid having to talk to "adb + shell" frequently. + + */ + + tasm->old_adb_detected = true; + return try_adb_stub(tasm, adb_args, adb_name, chello, err); + } + die_rethrow(&ei); + } + return info.ret; +} + struct delete_device_tmpfile { const char** adb_args; char* device_filename; @@ -351,17 +449,12 @@ static struct child* start_stub_adb(const char* const* adb_args, struct child_hello* chello) { - const struct child_start_info csi = { - .io[STDIN_FILENO] = CHILD_IO_PIPE, - .io[STDOUT_FILENO] = CHILD_IO_PIPE, - .io[STDERR_FILENO] = CHILD_IO_RECORD, - .exename = "adb", - .argv = ARGV_CONCAT(ARGV("adb"), adb_args, ARGV("shell")), - }; + struct try_adb_stub_memory tasm = { }; struct child* child = NULL; char* err = NULL; - child = try_adb_stub(&csi, FB_ADB_REMOTE_FILENAME, chello, &err); + child = try_adb_stub( + &tasm, adb_args, FB_ADB_REMOTE_FILENAME, chello, &err); if (child == NULL) { char* tmp_adb = xaprintf( @@ -378,7 +471,7 @@ start_stub_adb(const char* const* adb_args, #endif for (unsigned i = first_stub; i < ns && !child; ++i) { send_stub(stubs[i].data, stubs[i].size, adb_args, tmp_adb); - child = try_adb_stub(&csi, tmp_adb, chello, &err); + child = try_adb_stub(&tasm, adb_args, tmp_adb, chello, &err); } if (!child) @@ -393,7 +486,8 @@ start_stub_adb(const char* const* adb_args, api_level, ADB_RENAME_FALL_BACK_TO_CAT, adb_args); - child = try_adb_stub(&csi, FB_ADB_REMOTE_FILENAME, chello, &err); + child = try_adb_stub(&tasm, adb_args, + FB_ADB_REMOTE_FILENAME, chello, &err); if (!child) die(ECOMM, "trouble starting adb stub: %s", err); } diff --git a/util.c b/util.c index 0eb291c..02ef91c 100644 --- a/util.c +++ b/util.c @@ -1681,6 +1681,35 @@ grow_buffer_dwim(struct growable_buffer* gb) resize_buffer(gb, bufsz); } +void +growable_string_append_c(struct growable_string* gs, char c) +{ + assert(gs->strlen <= gs->gb.bufsz); + if (gs->strlen == gs->gb.bufsz) + grow_buffer_dwim(&gs->gb); + assert(gs->strlen < gs->gb.bufsz); + gs->gb.buf[gs->strlen++] = c; +} + +void +growable_string_trim_trailing_whitespace(struct growable_string* gs) +{ + while (gs->strlen > 0 && strchr(" \t\r\n\v", gs->gb.buf[gs->strlen - 1])) + gs->strlen--; +} + +const char* +growable_string_c_str(struct growable_string* gs) +{ + assert(gs->strlen <= gs->gb.bufsz); + if (gs->strlen == gs->gb.bufsz) { + grow_buffer_dwim(&gs->gb); + } + assert(gs->strlen < gs->gb.bufsz); + gs->gb.buf[gs->strlen] = '\0'; + return (const char*) &gs->gb.buf[0]; +} + static void cleanup_regfree(void* data) { diff --git a/util.h b/util.h index 1da7a32..f821389 100644 --- a/util.h +++ b/util.h @@ -372,6 +372,18 @@ void resize_buffer(struct growable_buffer* gb, size_t new_size); void grow_buffer(struct growable_buffer* gb, size_t min); void grow_buffer_dwim(struct growable_buffer* gb); +// Like growable_buffer, but deals in characters and automatically +// NUL-terminates + +struct growable_string { + struct growable_buffer gb; + size_t strlen; +}; + +void growable_string_append_c(struct growable_string* gs, char c); +const char* growable_string_c_str(struct growable_string* gs); +void growable_string_trim_trailing_whitespace(struct growable_string* gs); + regex_t* xregcomp(const char* regex, int cflags); char* xregerror(int errcode, const regex_t* preg);