Fix error "[child] expected 0x65 e, found 0x1b ." on some firmwares.

[dlitz]

This occurs when busybox is built with FEATURE_EDITING_ASK_TERMINAL, such as on TWRP recovery firmwares.

(Note that /system must be mounted before connecting to a recovery firmware with fb-adb.)

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[dcolascione]

Shouldn't chat_swallow_prompt already be responding to this query?

[dcolascione]

Ah. After the prompt.

[dcolascione]

Anyway, failing if we see any escape sequence that isn't exactly CSI 6 n feels wrong. How about repurposing the state machine we already have in chat_swallow_prompt to consume the entire sequence?

[dlitz]

I had a similar thought at first, but there doesn't appear to be any way to consume this escape sequence inside chat_swallow_prompt without introducing race conditions or a lot of needless complexity.

Immediately after we've received the prompt (i.e. after the space), we can't just chat_expect_maybe(ESC) because most firmwares wait for input after sending the prompt, so we'd be waiting forever.[1] We need to transmit at least one character after the prompt to see whether that character or an ESC is echoed back, and the most sensible place to do that seems to be inside chat_talk_at.[2]

Also, attempting to respond to that escape sequence is racy because of the way busybox is implemented, and also fundamentally because of the design of the *nix tty system. If you look at busybox's lineedit.c, you'll see what I mean:

#if ENABLE_FEATURE_EDITING_ASK_TERMINAL
static void ask_terminal(void)
{
    /* Ask terminal where is the cursor now.
     * lineedit_read_key handles response and corrects
     * our idea of current cursor position.
     * Testcase: run "echo -n long_line_long_line_long_line",
     * then type in a long, wrapping command and try to
     * delete it using backspace key.
     * Note: we print it _after_ prompt, because
     * prompt may contain CR. Example: PS1='\[\r\n\]\w '
     */
    /* Problem: if there is buffered input on stdin,
     * the response will be delivered later,
     * possibly to an unsuspecting application.
     * Testcase: "sleep 1; busybox ash" + press and hold [Enter].
     * Result:
     * ~/srcdevel/bbox/fix/busybox.t4 #
     * ~/srcdevel/bbox/fix/busybox.t4 #
     * ^[[59;34~/srcdevel/bbox/fix/busybox.t4 #  <-- garbage
     * ~/srcdevel/bbox/fix/busybox.t4 #
     *
     * Checking for input with poll only makes the race narrower,
     * I still can trigger it. Strace:
     *
     * write(1, "~/srcdevel/bbox/fix/busybox.t4 # ", 33) = 33
     * poll([{fd=0, events=POLLIN}], 1, 0) = 0 (Timeout)  <-- no input exists
     * write(1, "\33[6n", 4) = 4  <-- send the ESC sequence, quick!
     * poll([{fd=0, events=POLLIN}], 1, -1) = 1 ([{fd=0, revents=POLLIN}])
     * read(0, "\n", 1)      = 1  <-- oh crap, user's input got in first
     */
    struct pollfd pfd;

    pfd.fd = STDIN_FILENO;
    pfd.events = POLLIN;
    if (safe_poll(&pfd, 1, 0) == 0) {
        S.sent_ESC_br6n = 1;
        fputs(ESC"[6n", stdout);
        fflush_all(); /* make terminal see it ASAP! */
    }
}
#else
#define ask_terminal() ((void)0)
#endif

-- https://git.busybox.net/busybox/tree/libbb/lineedit.c?id=3e134ebf6afb5552b3619f98f6a2ffa01a07eebb#n1738

#if ENABLE_FEATURE_EDITING_ASK_TERMINAL
    if (S.sent_ESC_br6n) {
        /* "sleep 1; busybox ash" + hold [Enter] to trigger.
         * We sent "ESC [ 6 n", but got '\n' first, and
         * KEYCODE_CURSOR_POS response is now buffered from terminal.
         * It's bad already and not much can be done with it
         * (it _will_ be visible for the next process to read stdin),
         * but without this delay it even shows up on the screen
         * as garbage because we restore echo settings with tcsetattr
         * before it comes in. UGLY!
         */
        usleep(20*1000);
    }
#endif

-- https://git.busybox.net/busybox/tree/libbb/lineedit.c?id=3e134ebf6afb5552b3619f98f6a2ffa01a07eebb#n1738

Once busybox receives a newline, it waits for a response for only an additional 20ms before giving up and executing the command under the assumption that no response is forthcoming. (If that assumption is wrong, the response leaks into stdin of the program being executed.) For race-free behavior, we need to respond to the query either before sending any newline, or not at all.

(You'll also note that fputs(ESC"[6n", stdout) is hard-coded, so it's the only escape sequence that we need to consider here. Current behavior without this patch is to fail if we see anything that isn't an exact echo of the command we sent, so this patch just makes that check a bit more permissive.)

It turns out that we can avoid all of the races and most of the complexity by just ignoring the ESC"[6n" escape sequence if we happen to see it while we're entering a command into the prompt. It's a hack, but given the inherently broken design of the *nix tty system (in-band signaling; ugh), I don't think we can do much better without making the code more complex and/or less reliable.

Fundamentally, it seems to be impossible for chat_swallow_prompt to exist with the semantics that we'd like; we can't swallow all of the escape sequences in a race-free way without transmitting something that can be echoed back. To reflect this, it might also make sense to declare chat_swallow_prompt static and to add an extra flag to chat_talk_at that determines whether it tries to swallow the prompt. (I considered including that in my patch, but I wasn't sure whether such a change would be welcome here.) Nothing about this patch prevents that from being done after it's applied.

[1] Peeking at the input buffer is racy, and there's not really a good way to do it anyway because select/poll/etc work directly on filedescriptors, and we're using buffered stdio FILE streams, which provide no API for peeking at their input buffers at all.

[2] We could maybe transmit some sentinel and then backspace it, but that's surprising behavior for a function called chat_swallow_prompt, and backspace escape sequences are their own separate nightmare.

[dlitz]

Also, I just signed the CLA.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[dlitz]

I've added another commit, which turns chat_swallow_prompt into a private implementation detail of chat_talk_at, which accepts a new CHAT_SWALLOW_PROMPT flag. I don't have a strong preference about whether one or both commits are merged.

[lockheed]

So is there a chance to fix it? I am getting this error right now with the latest git version.

[mzlee]

Yep, sorry about the delay.

[dlitz]

Thanks for merging! :)