Refactor VT parameter handling

[j4james]

This PR introduces a pair of classes for managing VT parameters that
automatically handle range checking and default fallback values, so the
individual operations don't have to do that validation themselves. In
addition to simplifying the code, this fixes a few cases where we were
mishandling missing or extraneous parameters, and adds support for
parameter sequences on commands that couldn't previously handle them.
This PR also sets a limit on the number of parameters allowed, to help
thwart DoS memory consumption attacks.

References

• The new parameter class also introduces the concept of an
  omitted/default parameter which is not necessarily zero, which is a
  prerequisite for addressing issue The Resize Window escape sequence doesn't support omitted and zero parameters #4417.

Detailed Description of the Pull Request / Additional comments

There are two new classes provide by this PR: a VTParameter class,
similar in function to a std::optional<size_t>, which holds an
individual parameter (which may be an omitted/default value); and a
VTParameters class, similar in function to gsl:span<VTParameter>,
which holds a sequence of those parameters.

Where VTParameter differs from std::optional is with the inclusion
of two cast operators. There is a size_t cast that interprets omitted
and zero values as 1 (the expected behaviour for most numeric
parameters). And there is a generic cast, for use with the enum
parameter types, which interprets omitted values as 0 (the expected
behaviour for most selective parameters).

The advantage of VTParameters class is that it has an at method that
can never fail - out of range values simply return the a default
VTParameter instance (this is standard behaviour in VT terminals). It
also has a size method that will always return a minimum count of 1,
since an empty parameter list is typically the equivalent of a single
"default" parameter, so this guarantees you'll get at least one value
when iterating over the list with size().

For cases where we just need to call the same dispatch method for every
parameter, there is a helper for_each method, which repeatedly calls a
given predicate function with each value in the sequence. It also
collates the returned success values to determine the overall result of
the sequence. As with the size method, this will always make at least
one call, so it correctly handles empty sequences.

With those two classes in place, we could get rid of all the parameter
validation and default handling code in the OutputStateMachineEngine.
We now just use the VTParameters::at method to grab a parameter and
typically pass it straight to the appropriate dispatch method, letting
the cast operators automatically handle the assignment of default
values. Occasionally we might need a value_or call to specify a
non-standard default value, but those cases are fairly rare.

In some case the OutputStateMachineEngine was also checking whether
parameters values were in range, but for the most part this shouldn't
have been necessary, since that is something the dispatch classes would
already have been doing themselves (in the few cases that they weren't,
I've now updated them to do so).

I've also updated the InputStateMachineEngine in a similar way to the
OutputStateMachineEngine, getting rid of a few of the parameter
extraction methods, and simplifying other parts of the implementation.
It's not as clean a replacement as the output engine, but there are
still benefits in using the new classes.

Validation Steps Performed

For the most part I haven't had to alter existing tests other than
accounting for changes to the API. There were a couple of tests I needed
to drop because they were checking for failure cases which shouldn't
have been failing (unexpected parameters should never be an error), or
testing output engine validation that is no longer handled at that
level.

I've added a few new tests to cover operations that take sequences of
selective parameters (ED, EL, TBC, SM, and RM). And I've
extended the cursor movement tests to make sure those operations can
handle extraneous parameters that weren't expected. I've also added a
test to verify that the state machine will correctly ignore parameters
beyond the maximum 32 parameter count limit.

I've also manual confirmed that the various test cases given in issues
#2101 are now working as expected.

Closes #2101

[j4james]

I think this is ready for review now, assuming the build failure is just the CI bot having one of it's off days. I know this is a really big change, but I think it'll make things a lot easier for future additions to the VT code, and it does also fix a number of edge case scenarios.

I haven't included support for colon subparameters at this point, but I do have some ideas for how that could be achieved. I'm not entirely convinced it's a good idea, though, so I'm not in a hurry to add that functionality yet.

[DHowett]

I haven't included support for colon subparameters at this point, but I do have some ideas for how that could be achieved. I'm not entirely convinced it's a good idea, though, so I'm not in a hurry to add that functionality yet.

This is excellent work, as always. I've assigned Mike and Mike to dig in on the review front 😄

I'd be interested in hearing the broad-strokes outline of how we transition this into supporting subparameters! I ran into a couple gotchas when I was working on clump, like how \e[1:2;5:6H is totally rejected by xterm while gnome-terminal treats it like \e[H (with no specified parameters)

[github-actions]

New misspellings found, please review:

• TLambda

To accept these changes, run the following commands

perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"akb appdata Autogenerated CParams debian debugbreak decf DECLL DECSMBV esa guidgenerator Inplace keith keybound notypeopt openlogo restrictederrorinfo rgus richturn Scs SGRXY Switchto winver Wlk wslhome "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/3062d9af087f5711f5e0b6a72fcda6f336371161.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"autogenerated Debian inplace TLambda WINVER "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/3062d9af087f5711f5e0b6a72fcda6f336371161.txt is added to your repository...'

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

• ... misspelled, then please correct them instead of using the command.
• ... names, please add them to .github/actions/spell-check/dictionary/names.txt.
• ... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
• ... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
• ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

[j4james]

I'd be interested in hearing the broad-strokes outline of how we transition this into supporting subparameters! I ran into a couple gotchas when I was working on clump, like how \e[1:2;5:6H is totally rejected by xterm while gnome-terminal treats it like \e[H (with no specified parameters)

I'm undecided on the internals, but in terms of interface I'm expecting something like has_subparameters method on the VTParameters class which can be checked at the top of ActionCsiDispatch. If it indicates that there are any subparameters in the sequence, and it's not an SGR op, we can immediately fail, and thus won't have to worry about subparameters for any of those operations.

Then in the SetGraphicsRendition implementation we're going to need to test each parameter individually, so we'll also need a has_subparameters method on the VTParameter class. If that's set, and it's not one of the extended colors, we either abort immediately, or skip the subparameters. If it is an extended color parameter, we'll need something like a subparameters method that returns a new VTParameters span with the appropriate range, which can be passed to _SetRgbColorsHelper.

I wouldn't recommend paying much attention to the VTE implementation for stuff like this. They're not really a proper terminal emulator, so they often won't get details like this correct. According to the DEC documentation "The bit combination 3/10 is reserved for future standardization. If 3/10 is received within a parameter string, the entire sequence up to and including the final character shall be ignored." So what XTerm is doing here is correct.

Obviously we can't follow that rule in SGR if we want to support extended color colons, but SGR should be a one-off special case. And frankly I'm not even sure we should be supporting the colon syntax there. At this point it's clear the semicolon syntax has won the standard battle for the extended color format. Nearly everyone supports it, and hardly anyone supports colons, so no sane app developer would knowingly choose the syntax that doesn't work 90% of the time.

So while I kind of want to support colons for the sake of completeness, I don't think it's something we should be encouraging if we care at all about the broader ecosystem outside our userbase. Some have even suggested that terminals supporting the colon syntax should actually consider dropping it, and I'm somewhat in agreement with that sentiment.

[DHowett]

I don't disagree with much of that, but I also am not sure that DEC gets the final say 😄

ITU T.416 seems to standardize the bit combination 3/10 that was previously reserved. Is it not valid?

I'm interested in kitty's extension of underline SGR 4 to support styles, as well: they seem to have used subparameters, but there's no formal specification around it. I don't want to impede progress towards better graphics renditions, but I also don't want us to go blaze our own trail.

Anyway, we can talk about this at #4321 if need be 😄

[skyline75489]

I've browsed all the files and it's just beautiful. Can you trigger the CI by force pushing or merging master? The CI was messed up then and has been fixed now I believe. But you need to trigger it or the check will always be zero.

Wait "was messed up and has been fixed"? Is this correct? I need some English lesson, like right now.

[j4james]

Your English seems perfect to me! 😄

And I'm not worried about it not building. I'll rebase if necessary when there's a conflict with some other PR. Otherwise I'm sure one of the core devs will kick off a build when they get a chance to review it.

[DHowett]

/azp run

(??????? wut)

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[DHowett]

I'm not sure where on the barometer to place this, but it took me a little while to remember that whether we produce a 0 or a 1 is based on the receiving type. _GetModifier takes a size_t, so gets a 0. Discriminated types, as you called out, get a 1. If somebody were to change the type _GetModifier accepts, all this code would silently continue working (so long as the type it accepts meets sizeof(T) == sizeof(size_t)) but produce the wrong value.

Is that a concern? Or, am I overindexing on the concern?

[DHowett]

Okay, actually, I got the 0s and 1s backwards. I'm quite confused now -- does _GetModifier get a 0 here? Or it looks like it'll get a 1 because "omitted numeric parameters are 1 if 0 or omitted"? I'm confounded!

[DHowett]

Okay, I finally get it. No modifier is 1, presumably to support this exact behavior.

[j4james]

Actually, I think you've caught a bug. This is one of the few cases where you've got a "numeric" parameter that shouldn't default to 1. If I'm reading the docs correctly, 0 is button 1 and 1 is button 2.

But this is why I have the value_or method, so we can override the default value when necessary. Ideally that'll be very rare, but it can be a problem with proprietary sequences that weren't developed by DEC. I still think it's worth have the default just work automatically 90% of the time.

[DHowett]

Looking into it a bit more... _GetModifier subtracts 1 as the first thing it does; I believe now that this should default to 1. Otherwise modifier ends up being -1 before we start pulling flags out of it.

[j4james]

If somebody were to change the type _GetModifier accepts, all this code would silently continue working

This is a valid concern, but I think in most cases this should be picked up by the unit tests. The input state machine doesn't have unit tests for stuff like this because it never supported default parameters before. I probably should add some.

[j4james]

Looking into it a bit more... _GetModifier subtracts 1 as the first thing it does

Yeah, sorry, I was looking at the wrong modifier. For some reason I thought we were talking about the mouse modifiers. But the mouse code already does the value_or(0) thing when processing the button parameter, so I think we're all good.

[DHowett]

Not a line out of place, as always. This is really excellent work, @j4james.

Thank you.

[DHowett]

Incidentally, this shaves 1kb off a release build of OpenConsole 😄

[skyline75489]

Only one conflicting file, that's better than I thought. @j4james Hope it isn't too much trouble for you 😅

[zadjii-msft]

Alright so that doesn't look like your fault. Seems the "super linter" is turned on for C++ files, which I thought it explicitly didn't do. We'll have to get that sorted out. Sorry for the delay :/

[j4james]

Yeah. I'm guessing it just lints files included in the PR. So when the linter PR was added, it looked like it was only linting yaml, because that was the only file added. If it would help, I could rebase against the commit just prior to the lint PR? That should still resolve the merge conflict without the lint errors.

[zadjii-msft]

If it would help, I could rebase against the commit just prior to the lint PR? That should still resolve the merge conflict without the lint errors.

meh, it'll need to be fixed eventually, and if it's blocking this PR then it's more likely that @DHowett will take a look at it 😝 I think I've got a fix over in #7930, so shouldn't take that long.

[ghost]

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download