windows 10+: make 256/true colors "work" without -Da

[avih]

See the commit message for full description.

TL;DR:

Before commit f9f2498 (which is just before v651), unknown SGR values resulted in aborting the SGR parsing and instead passthrough till the rest of the buffer to the terminal directly.

This had broken output if the terminal doesn't support VT (e.g. win7), but seemingly correct output with VT, e.g. 256 colors on win10.

Commit f9f2498 changed this process, by ignoring only the rest of the current sequence if an unknown value is encountered, so that the next sequences in the buffer are still processed normally.

Additionally, we then never lose track of the terminal state, because either we recognize a value and apply it, or we do neither of those. Specifically, we never apply a value which we can't recognize/track.

While it fixed the junk output when VT is disabled, it also had an effect which made 256 and true colors stop "working" on win10+.

So with the latest release v661, it can be observed than 256 and true colors stopped working compared to the previous release v643.

So this commit brings back this behavior of "working" 256/true colors on win10+ by default, but a bit more refined than before f9f2498 .

Fixes #538

[avih]

Forced pushed with refined commit message. No code changes.

[avih]

Rewrote the commit message to explain how v661 can be considered a regression compared to v643 in some cases, and how this commit does the best of both worlds to a degree which it appears perfect.

[avih]

So, I started doogfooding master just now, and noticed that there's an issue, possibly related to this commit.

Less crashes with this file (16 lines true color "plane") before completing two lines - true-color.zip .

Each line is ~ 1400 bytes of true color escape sequences, and ends in SGR reset.

I suspect a buffer overflow issue which got exposed by this PR, but I don't have more details currently.

I'll debug and post updates here.

Meanwhile, please wait withthe release.

[avih]

The offending commit is mine - f9f2498 just before v651.

Debugging.

[avih]

OK, I still don't know why it crashes, but this is not a new issue.

It was exposed by this part of the diff of commit f9f2498 :

@@ -299,16 +312,20 @@ static void win_flush(void)
                                        }

                                        if (q == p ||
-                                               code > 49 || code < 0 ||
                                                (!is_ansi_end(*q) && *q != ';'))
                                        {
+                                               /*
+                                                * can't parse. passthrough
+                                                * till the end of the buffer
+                                                */
                                                p_next = q;
                                                break;
                                        }

I.e. this code aborts the parsing and switches to passthrough based on the inaccurate test of code > 49 || code < 0.

And that commit ignores the rest of the sequence instead of switching to passthrough.

I tested also with commit 52b8e35 - which is 1 commit before we started touching win_flush (few years old parser code). By default it doesn't crash, but if I comment out that code > 49 || code < 0 || line then it crashes.

I still don't know why, but I suspect it's an edge case bug which my new SGR parser fixes (it doesn't crash with my own from-scratch parser, and this is why I haven't noticed it, because in my build I use my parser, but I wanted to dogfood master a bit before your release, and I discovered it).

@gwsw any thoughts?

I still have a hunch about buffer overflow, but that test line (>49, <0) is just a trigger. There's nothing special about these values, and these thresholds are arbitrary, and there are unrecognized numbers even within this range (like 38), and the code implicitly does SGR reset in such case - it assumes any unrecognized value must have been 0:

    default:
    /* case 0: all attrs off */

So there's nothing special about values outside this range which would trigger bad behavior.

Amd I think it has to be the fact that it doesn't abort early, combined with this specific input.

But I don't yet know why that results in a crash.

[avih]

It's "fixed" If I change:

static char obuf[OUTBUF_SIZE];

into

static char obuf[OUTBUF_SIZE * 2];

I.e. from 1024 to 2048 if I got the numbers right.

But it's not related to 1024 specifically.

If I change it to half the size instead (512), then it still goes further and completes almost 4 lines before crashing.

So there's some math or behavior error depending on how the input length stars align.

[avih]

OK, I sort of got it.

This loop:

		for (anchor = p_next = obuf;
			 (p_next = memchr(p_next, ESC, ob - p_next)) != NULL; )

can be entered (I'm guessing not in the first iteration) when p_next is past the end of the buffer (ob), and then all bets are off.

Now only remains to find the offensive asumption or calculation inside that loop.

This shouldn't take long.