[Toybox] Junk printed on exit from top

[Rob Landley]

On 8/28/20 12:28 PM, Rich Felker wrote:
> With toybox 0.8.3 on J2 I'm getting some junk printed along with the
> final escape sequence when exiting top. strace shows:
> 
> writev(1, [{"\0\0\0\1\0\0\0\1\24\307\252\364\24\274!`\24\277\320\220\0\0 \0H\33[K", 28}, {NULL, 0}], 2) = 28

Toybox doesn't call writev() directly so I'm guessing that's how musl is
implementing printf() under the covers.

On exit, top_main() in ps.c is calling:

void tty_reset(void)
{
  set_terminal(0, 0, 0, 0);
  tty_esc("?25h");
  tty_esc("0m");
  tty_jump(0, 999);
  tty_esc("K");
  fflush(0);
}

That's the only string constant ending with K in ps.c or lib/*.c, by the way:

$ grep 'K"' toys/*/ps.c lib/*.c
lib/tty.c:  tty_esc("K");

So no _other_ codepath could produce the valid output at the end.

The set_terminal() call doesn't write anything to stdio, that function is
basically a tcgetattr()/tcsetattr() pair with a bunch of struct micromanaging in
between.

Those last two functions are:

void tty_jump(int x, int y)
{
  char s[32];

  sprintf(s, "%d;%dH", y+1, x+1);
  tty_esc(s);
}

void tty_esc(char *s)
{
  printf("\033[%s", s);
}

The above output ending in "\0\0 \0H\33[K" has the last 4 characters right, but
a NULL right before that? The source of the data is sprintf() passed to printf()
via a very short function call chain that's getting the tail of the data right.
So is the garbage coming out of %d from sprintf, or being overwritten after the
sprintf() happens?

It looks like there's 24 bytes before the H, which is still comfortably in the
s[32] buffer allocation. (Which _can't_ overflow, -2 billion is 11 characters,
22+3=25 bytes, then the allocation's rounded up to a power of 2 just because.)

This H is 24 bytes in, which is weird. If we somehow overwrote successful
sprintf() output in s[], how the heck did we get an offset of 24? The sprintf()
would have to have written 24 bytes of garbage into s[] to get the H that far to
the right, which is the longest possible output, I.E. both y and x were a
negative value over 1 billion, and THEN the output would have to get stomped
with binary garbage instead of being a number (no matter what the _input_ to
sprintf() is, the output should be digits). There's no \33[ earlier in the
writev() output either, and no matter what s[] points to, the printf() should
start the output with that. This means that the failure isn't corrupted _input_
to the printf call (unless the string pointer itself got borked somehow and the
trailing H is coincidental? Again, seems unlikely, it's a string constant passed
directly to the function...)

Reconstructing what the output SHOULD be...

  \33[?25h\33[0m\33[1;1000H\033[K

Which puts the H at an offset of 18, not 24. But then none of the previous
output is validly concatenated into the writev() either, and we haven't had a \n
or a flush() so...

The code that's calling all this is doing (on exit):

      if (FLAG(b)) {
...
      } else fflush(stdout);

      i = scan_key_getsize(scratch, timeout-now, &TT.width, &TT.height);
      if (i==-1 || i==3 || toupper(i)=='Q') {
        done++;
        break;
      }

We know -b isn't supplied because it wouldn't call tty_reset(0 if so, so it's
going to fflush(stdout) so whatever happens shouldn't be in the same writev() as
previous stuff. scan_key_getsize() doesn't write anything (the getsize is
because it can parse the return from a previous terminal_probesize(), which
would be before the flush). When it gets a Q or ctrl-C or EOF, it exits pretty
much immediately, down to:

    }

    free(mix.tb);
    for (i=0; i<plold->count; i++) free(plold->tb[i]);
    free(plold->tb);
  } while (!done);

  if (!FLAG(b)) tty_reset();
}

I.E. it then calls free() a few times after the inner loop, and then while
(!done) exits the outer loop, and the next line is the tty_reset() on the way
out. (Even if those free() calls were spurious, I don't think anything does no
mallocs() afterwards? Unless tcgetattr/cfmakeraw/tcsetattr call it internally,
or sprintf/printf does...)

That's the context for this. We call sprintf() on a (valid!) stack buffer which
calls printf() which writes data into the stdout buffer, then we call fflush(0)
which passes corrupted data to writev(). The result written out is not the right
LENGTH, as far as I can tell...

> I haven't tracked down the cause. I did look for relevant commits
> since then that migth have fixed it but didn't see any so I haven't
> retested with latest.

I'm not seeing it on x86-64. What toolchain build are you using?

> Rich

Rob