alacritty / vte

Parser for virtual terminal emulators
https://docs.rs/vte/
Apache License 2.0
242 stars 56 forks source link

Use dynamically sized buffer for OSC #20

Closed wez closed 5 years ago

wez commented 5 years ago

Whilst playing around with vte and processing iTerm2's image protocol, I ran into the relatively small statically sized OSC buffer.

This diff switches the buffer from a static array to use a Vec<u8>. I did briefly experiment with just making this buffer much larger (as https://github.com/jwilm/vte/pull/15 does), but even at 1MB (which is too small for images) that size can lead to a stack overflow during initialization.

In order to preserve building in a no_std environment this is done conditionally. I've followed the pattern used in other crates with similar requirements:

Test Plan:

$ cargo test --no-default-features
$ cargo test

Refs: https://github.com/jwilm/alacritty/issues/1002

jwilm commented 5 years ago

Thanks for the great PR; I love the approach here! One of the things that was nice about the fixed-size buffer was that a misbehaving terminal application couldn't cause the vte process to allocate an unlimited amount of memory.

How do you think about having an "unlimited" sized buffer as in this PR? On one hand, it's nice being able to opt into a limit to protect resources and other processes on the machine (ugh, oom-killer). On the other hand, TEs are usually running trusted applications, and the main avenue for excessive consumption would be through misuse of an application.

wez commented 5 years ago

I suspect that the most likely cause of a problem with this is going to be a programmer error rather than something malicious; when I think of malicious terminal exploits I think largely of nefarious traps in web pages when using copy and paste, rather than running an untrusted application via the terminal--in the untrusted application scenario, the primary problem is the running of that application.

Having a limit to protect against something stupid doesn't sound unreasonable, but I can also see it potentially getting in the way. For example, the iTerm image protocol is also able to be used to transfer files of arbitrary size (https://www.iterm2.com/documentation-images.html) so setting the limit too small could break this.

Here's what I suggest: introduce a configurable limit that can be set when constructing the parser. The default could be say u32::max_value() to set a ballpark upper bound and allow the parser to eventually make progress. That limit is big enough for a lot of things and not too big that it would on its own exhaust all the memory on the machine, and hopefully keep things under the radar from eg: oom killer.

We could also potentially augment the clear call to resize the vec back down to 1k if it is above some threshold to keep the heap size better constrained.

jwilm commented 5 years ago

A configurable max sounds good to me. I also like the idea of freeing the memory after use. Thanks for the feedback!

wez commented 5 years ago

bump

jwilm commented 5 years ago

Hey @wez, thanks for the bump, and sorry for the delay here. I'm in the process of moving this week won't be able to get back to this for at least a week or two.

@chrisduerr may be able to take a peak at this sooner than that (no promises).

Thanks for your patience.

chrisduerr commented 5 years ago

I can add some comments if a review is desired, however I don't have access to this repo so I can't merge it if it does not have any issues.

wez commented 5 years ago

friendly ping :)

chrisduerr commented 5 years ago

I'm currently busy with https://github.com/jwilm/alacritty/pull/2438 which will probably take another week.

I have this tracked, but it will take some time until I can take a look at it.