Translate non-UTF-8 byte sequences into replacement characters

[sunfishcode]

This is a work in progress; I'm posting it now to provide context for https://github.com/alacritty/vte/pull/58.

This patch series fixes all the cases I could find where vte would silently discard bytes in the presence of invalid UTF-8 byte sequences, and makes it emit replacement characters in a manner compatible with Rust's String::from_utf8_lossy and the resolution to https://hsivonen.fi/broken-utf-8/, and fixes #38.

There are other ways I could achieve my goals here. Building this patch series was useful for me to understand all the different aspects of this. But if taking a different approach would be better, I'm open to ideas.

[sunfishcode]

I've now added a way to disable recognition of DCS, SOS, PM, and APC sequences, since they are all terminated by 8-bit ST, which isn't valid UTF-8. They are still recognized by default, but the patch adds a flag for optionally disabling them.

This is now ready for review feedback. There are several changes here, though they are split up into self-contained commits. I'm curious whether these kinds of changes are in scope for vte, and whether the approach I took here makes sense.

[kchibisov]

Have you run benchmarks against your changes? You can do that with cargo bench --features=nightly and compare them to master.

[chrisduerr]

I've now added a way to disable recognition of DCS, SOS, PM, and APC sequences, since they are all terminated by 8-bit ST, which isn't valid UTF-8.

I don't quite understand what you mean and having a flag to disable these doesn't make much sense to me for a vt parser. Correctly parsing escapes is the one thing we're supposed to do.

First of all, only because an escape is terminated with ST, doesn't mean it has to be the C1 ST. The C0 one should work perfectly fine. Even if ST is listed, BEL often works just as well.

All C1 control characters are also valid UTF-8. So I have absolutely no idea what you mean by this.

[sunfishcode]

@kchibisov

Have you run benchmarks against your changes? You can do that with cargo bench --features=nightly and compare them to master.

There's currently about a 20% slowdown in that benchmark as a result of this PR. That said, that test contains quite a lot of byte sequences which are not valid UTF-8, and with this PR, those sequences are translated into replacement characters rather than being ignored, so it's doing more work. How important is performance on binary or otherwise non-UTF-8 input for vte?

On test files containing valid UTF-8, it ranges from having no slowdown to about a 5% slowdown, depending on the content. I'll take a look to see if there are any opportunities to optimize.

@chrisduerr

I don't quite understand what you mean and having a flag to disable these doesn't make much sense to me for a vt parser. Correctly parsing escapes is the one thing we're supposed to do.

Some users will want to parse DCS, SOS, PM, and APC. But many terminals these days don't do anything with DCS, and I don't know of any that do anything with SOS, PM, or APC. So if these codes interfere with things that other users want, it doesn't seem unreasonable to have a way to disable them.

First of all, only because an escape is terminated with ST, doesn't mean it has to be the C1 ST. The C0 one should work perfectly fine. Even if ST is listed, BEL often works just as well.

All C1 control characters are also valid UTF-8. So I have absolutely no idea what you mean by this.

In a C0 ST, the ESC cancels the sequence, before the \ is seen, so it doesn't work the same way.

When they extended OSC to recognize BEL instead of ST, I don't know why they didn't do the same for DCS, SOS, PM, or APC. But it seems they didn't; I can't find anything that recognizes BEL for those.

vte doesn't recognize the UTF-8 C1 encodings as control codes. In the SosPmApcString state, 0xC2 defaults to being ignored. A subsequent 0x9C looks like an 8-bit C1, which is what we're trying to avoid.

[chrisduerr]

How important is performance on binary or otherwise non-UTF-8 input for vte?

Performance is very important.

On test files containing valid UTF-8, it ranges from having no slowdown to about a 5% slowdown

A slowdown of 5% for input that isn't affected at all would probably block this PR. Though it depends a bit on how the performance is in Alacritty. But that's quite a big slowdown.

In a C0 ST, the ESC cancels the sequence, before the \ is seen, so it doesn't work the same way.

It doesn't work the same way, but it still works. So why would we remove support for things with no benefit of doing so? If the goal is only to remove C1 support, we could still support it with C0 just fine?

[sunfishcode]

How important is performance on binary or otherwise non-UTF-8 input for vte?

Performance is very important.

My question was about binary or otherwise non-UTF-8 data specifically. If that's important, then it's unlikely that any amount of optimization will fix it, because the point of this PR is to do more work in those cases.

A slowdown of 5% for input that isn't affected at all would probably block this PR. Though it depends a bit on how the performance is in Alacritty. But that's quite a big slowdown.

I haven't spent much time on optimization yet, so it's possible there are opportunities to improve performance. However, it sounds like you're not interested in this PR, so I'll look into other options.

In a C0 ST, the ESC cancels the sequence, before the \ is seen, so it doesn't work the same way.

It doesn't work the same way, but it still works. So why would we remove support for things with no benefit of doing so? If the goal is only to remove C1 support, we could still support it with C0 just fine?

It isn't just about whether there exists a byte sequence which has the desired effect; it's also about what applications are emitting, and if they make a change, how that change would work in other terminals they work in. From what I can tell, C0 ST causes DCS strings to be cancelled in other terminals, so if existing applications using DCS with C1 ST are going to make any change at all, it'd make more sense for them to stop using DCS altogether. Since DCS is obscure and not widely supported these days anyway, moving away from it in general seems to make more sense.

[chrisduerr]

My question was about binary or otherwise non-UTF-8 data specifically. If that's important, then it's unlikely that any amount of optimization will fix it, because the point of this PR is to do more work in those cases.

Of course the performance of that matters too, but if there's no effect on other cases a small hit is fine there. It's not necessary to have absolutely perfect performance when it comes to parsing invalid input.

I haven't spent much time on optimization yet, so it's possible there are opportunities to improve performance. However, it sounds like you're not interested in this PR, so I'll look into other options.

I haven't looked at the PR, I'm mostly curious about your motivation and methodology. Just to be absolutely clear that my statements were purely about what you said, not the code itself.

It isn't just about whether there exists a byte sequence which has the desired effect; it's also about what applications are emitting, and if they make a change, how that change would work in other terminals they work in. From what I can tell, C0 ST causes DCS strings to be cancelled in other terminals, so if existing applications using DCS with C1 ST are going to make any change at all, it'd make more sense for them to stop using DCS altogether. Since DCS is obscure and not widely supported these days anyway, moving away from it in general seems to make more sense.

I'm not entirely sure what you're trying to say here. Are you saying that since C0 ST isn't supported by other terminals, applications that use the C1 ST have no reason to make any changes? I don't see why applications would make any change in the first place, as a parser we should support C1 ST just fine if the terminal chooses to use it. I don't see many reasons other than performance to drop it and based on my testing there is no performance benefit to dropping C1 support completely. Though we don't have C1 ST support for OSC either, which was necessary to fix a bug.

[sunfishcode]

I'm not entirely sure what you're trying to say here. Are you saying that since C0 ST isn't supported by other terminals, applications that use the C1 ST have no reason to make any changes? I don't see why applications would make any change in the first place, as a parser we should support C1 ST just fine if the terminal chooses to use it. I don't see many reasons other than performance to drop it and based on my testing there is no performance benefit to dropping C1 support completely. Though we don't have C1 ST support for OSC either, which was necessary to fix a bug.

For my use cases, I'm looking for a parser in which every invalid UTF-8 byte sequence can be replaced with U+FFFD. If the input contains a 0x9C anywhere that isn't in a UTF-8 continuation position, I'm going to replace it, so I won't recognize it at ST.

To me, 8-bit C1 codes mean increased surface area and weaker invariants. Being able to say that every non-UTF-8 byte sequence gets translated to U+FFFD means I can categorically rule out a class of potential bugs, such as the one you mentioned. One of the properties of UTF-8 is that it's self-synchronizing, so if I ever have to start decoding in the middle of a stream, I can always at least know where the code-point boundaries are, but that property doesn't completely hold in "UTF-8 with 8-bit C1 codes". And, some terminal implementations today do UTF-8 decoding as an independent step before escape-sequence decoding, so even if I'm using a library that doesn't do that today, I want the option of doing so in the future without having to worry about backwards compatibility.

Some of these risks are arguably minor, but the 8-bit C1 codes sufficiently obscure and insufficiently useful that, for my purposes at least, they not worth it.

[chrisduerr]

Being able to say that every non-UTF-8 byte sequence gets translated to U+FFFD means I can categorically rule out a class of potential bugs

I don't see how the user of VTE would be able to rule out any bugs with the help of that. It's just dropped vs replaced for them.

Some of these risks are arguably minor, but the 8-bit C1 codes sufficiently obscure and insufficiently useful that, for my purposes at least, they not worth it.

The thing is that vte is a library, so the goal is to try and accommodate for a wide range of usecases, not just the one specific to Alacritty for example. We personally have removed all support for C1 escapes too, but I'm not sure if I want to do that to vte.

Maybe the current downstream users of vte should also be evaluated. If it really causes so much overhead and nobody actually uses it, maybe giving up on C1 is the better option. There's currently not really proper C1 support in vte, so maybe we're just making things worse for everyone by clinging to some halfway state that doesn't help anyone who wants C1 support but holds everyone else back.

I know that other terminals print replacement characters, are you aware of how these handle C1 escapes?

[sunfishcode]

I don't see how the user of VTE would be able to rule out any bugs with the help of that. It's just dropped vs replaced for them.

It would be simpler if there were a spec to follow. While the VT100 state machine diagram is a great starting point, it doesn't incoporate UTF-8. VTE is currently making its own choices about when and where to support 8-bit C1 controls. Will these choices be compatible with the choices my users are making? If not, is it my (or VTE's) bug or theirs? If I switch to a different parser, will I have subtle incompatibilities?

Maybe the current downstream users of vte should also be evaluated. If it really causes so much overhead and nobody actually uses it, maybe giving up on C1 is the better option. There's currently not really proper C1 support in vte, so maybe we're just making things worse for everyone by clinging to some halfway state that doesn't help anyone who wants C1 support but holds everyone else back.

I agree, this sounds like a good evaluation to do.

I know that other terminals print replacement characters, are you aware of how these handle C1 escapes?

According to Wikipedia, 0x85 the 8-bit C1 code might be still in use. I checked 9 different virtual terminals, plus screen and tmux, and the behavior ranges from silently ignoring it, printing ▒, printing �, and one implementation which supports it, which is xterm, and only in its non-UTF-8 mode.

[chrisduerr]

According to Wikipedia, 0x85 the 8-bit C1 code might be still in use.

Based on my testing, there's enough that don't support it that you cannot rely on it. Which seems in line with what you've seen. IIRC by default neither URxvt, VTE, Kitty nor XTerm support it.

So I'd say there are two questions mainly:

• If we decide that it should be possible to support C1 escapes with VTE, should it be possible at runtime or only at compile-time
• Should we support C1 escapes at all

Since you've looked into this already, you can probably best evaluate what overhead a runtime configuration would have. Do you think that it would be any slower than a compile-time option?

And is there any performance advantage to completely dropping C1 support, or does the compile-time option only add code complexity without affecting performance?

[sunfishcode]

Right now, it looks like VTE's state machine is a fairly literal translation of the DEC state machine diagram, with a separate state machine for UTF-8. As it is now, removing 8-bit C1 support probably wouldn't help performance much.

There are several optimizations possible. For example, when in the Utf8 state and a 0x80 arrives, it should be able to dispatch directly into the code for handling UTF-8 continuation bytes. Or when in the Ground state and a 0x80 arrives, it should be possible to dispatch directly into the code for handling an invalid UTF-8 encoding. These kinds of optimizations would be either less effective or more complex, or both, with 8-bit C1 support.

The runtime option I experimented with for disabling DCS/SOS/PM/APC looks to have caused about a 5% slowdown. It seems likely that a runtime option to disable/enable 8-bit C1 support would be similar.

Should we support C1 escapes at all

Of course, this is just my opinion, but for what it's worth, I suggest "no" here, unless you know of a specific user that needs it. UTF-8 is so important in so many contexts these days, it should take a really strong justification to support exceptions to UTF-8 in a protocol which is otherwise UTF-8.

As an update to a comment I made earlier, it appears XTerm in UTF-8 mode (the default mode in many OS's today) does recognize ESC \ as a string terminator for DCS/SOS/PM/APC, and doesn't recognize 0x9C. With that, it's possible to support the full feature set including DCS etc. within a valid UTF-8 bytestream, and it's compatible with XTerm. So for what it's worth, that's the path I think that makes the most sense.

[chrisduerr]

There are several optimizations possible. For example, when in the Utf8 state and a 0x80 arrives, it should be able to dispatch directly into the code for handling UTF-8 continuation bytes. Or when in the Ground state and a 0x80 arrives, it should be possible to dispatch directly into the code for handling an invalid UTF-8 encoding. These kinds of optimizations would be either less effective or more complex, or both, with 8-bit C1 support.

That's a good point and I've considered this myself in the past. It seems like there would be some significant changes necessary, but with a tiny lib like vte that shouldn't be a massive problem either.

Of course, this is just my opinion, but for what it's worth, I suggest "no" here, unless you know of a specific user that needs it. UTF-8 is so important in so many contexts these days, it should take a really strong justification to support exceptions to UTF-8 in a protocol which is otherwise UTF-8.

I think I mostly agree at this point. Especially with the primary user of VTE not using C1 escapes at all. Looking at the RedOX and strip-ansi-escape usages of VTE it seems like they do the same. So that does raise the question why we should bother with C1 escapes at all when we can make our library better for all currently known users by dropping it.

As an update to a comment I made earlier, it appears XTerm in UTF-8 mode (the default mode in many OS's today) does recognize ESC \ as a string terminator for DCS/SOS/PM/APC, and doesn't recognize 0x9C. With that, it's possible to support the full feature set including DCS etc. within a valid UTF-8 bytestream, and it's compatible with XTerm. So for what it's worth, that's the path I think that makes the most sense.

That does seem to be the most reasonable solution, yes.