GNOME Bugzilla – Bug 730760
Data corruption immediately after a reset
Last modified: 2014-05-29 11:09:11 UTC
perf/img.sh [some_picture] > picture.txt echo -ne '\ec'; cat picture.txt Notice that the terminal contents are usually corrupted. (Scroll back to see it if necessary.) Highlight with mouse to see that part of an escape sequence is printed literally, e.g. "5;254;255;48;2;254;255;254m" This only happens if the reset is immediately followed by some data. Apparently some bytes in the input buffer get lost, maybe due to improper initialization of the new charset converter or iso2022 state machine, or by dropping the data from the old one, or such. (On a related thought, we should also verify that if a charset changing escape sequence is immediately followed by some input bytes, those are already interpreted according to the new charset.)
Created attachment 277218 [details] [review] simple proof of concept Simple patch. It's okay to drop the outgoing buffer (i.e. keypresses), but why drop the incoming one?
Created attachment 277226 [details] [review] more complex proof of concept There are two problems with reset: first, we drop the incoming buffer (why?), second: process_incoming() performs charset conversion on a large chunk of data and then processes its escape sequences, including reset, but then keeps processing the (now dropped) input buffer, I don't understand why it doesn't crash :) I'm quite certain that charset conversion indeed doesn't work immediately but with a latency, potentially causing hard to track heisenbugs. Here's a proof of concept patch that shows that handling reset (and charset change too, that's not yet implemented) should abort the loop and restart from the very beginning. There are other suspiciously cached variables in process_incoming(). The screen is stored, whether we're in a scrolling region is cached etc... I believe that if in a single chunk you modify the active screen, change the scrolling region and do stuff that depend on these, you might face bugs. It's not clear to me yet what would be the nice solution. As for in_scroll_region, now that I understand the code better I think that it shouldn't be here but rather somewhere in vteseq (find where we actually forget to invalidate cells, rather than work it around here). Also there's no point in cleverly trying to find one single bounding rectangle, invalidate_cells() already does it. As for screen and bottom, it's probably not a big deal if we're changing screen, since then the scrollbar is moved to bottom anyways. As for cursor, it seems it's only used for visual update and we only care about the difference in the initial and final value, so I guess we're okay. The only thing remaining is unichars (the result of charset convertion and iso2022 - kinda). I'm still thinking about how to solve this nicely.
Looks like charset change is handled explicitly already, somewhere else than I thought it'd be handled :) I guess we're safe to go with the first patch for now.
Comment on attachment 277218 [details] [review] simple proof of concept 9240ed8
Closing for now. The simple fix is good enough. The situation is not as bad as I outlined in comment 2, probably there are no more bugs left, making the refactoring very low prio.