GNOME Bugzilla – Bug 748484
crash in _vte_boa_reset
Last modified: 2015-04-28 21:58:40 UTC
From https://retrace.fedoraproject.org/faf/reports/591229/ : #3 g_assertion_message_cmpnum at gtestutils.c:2404 #4 _vte_boa_reset at vtestream-file.h:851 #5 _vte_file_stream_reset at vtestream-file.h:1083 #6 _vte_ring_reset_streams at ring.c:324 #7 _vte_ring_drop_scrollback at ring.c:637 #8 _vte_terminal_drop_scrollback at vte.c:3026 #9 vte_sequence_handler_erase_in_display at vteseq.c:2597 #10 vte_terminal_process_incoming at vte.c:3827
Oops. assertion failed (offset >= boa->head): (0 >= 65512) Will take a look.
Created attachment 302389 [details] [review] v0 (wip) It's tricky. So, there are two relatively rare special actions you can do on the stream: - truncate (that is, retreat the head, undo the last appends; this happens when the window is made taller). This is handled as a private business of vte_stream, it doesn't notify boa. What happens instead is that later the boa layer sees an overwrite of a block that it has written previously, and then it knows to increment the IV for security. - reset (as done by the "reset" command, kinda start everything from scratch). Again to prevent reuse of IV, the logical tail & head positions are not reset to 0, but instead set to the current head (that is, we move forward and leave everything behind for good). Boa and Snake are immediately notified and they also adjust their tail & head values. What causes the assertion failure is a truncate across a block boundary, followed by a reset inside that block. In that case, the position we try to reset to is smaller than boa's head. So here the assumption was incorrect. Just removing the assumption fixes that, but leads to IV reuse. (That's where the current patch is.) I can see two ways to continue. Either never retreat the head of the boa & snake layers, or reset to the high water mark of head rather than the current value...
Created attachment 302391 [details] [review] v1 (wip) This is not the cleanest part of vte... I'll see if I can simplify.
(In reply to Egmont Koblinger from comment #2) > - reset (as done by the "reset" command, kinda start everything from > scratch). Again to prevent reuse of IV, the logical tail & head positions > are not reset to 0, but instead set to the current head Correcting myself: set to a value that's at least the current head, perhaps even bigger. Life would be much easier if was always the current head. See also bug 748492.
The idea for exponential growth is that if you start resizing, we don't have to resize / copy array at every one additional row. Other than that, your proposal looks good to me. I believe you had proposed this same thing a while back, but guess you never got to implement it.
Comment was meant for bug 748492.
Created attachment 302415 [details] [review] v2 (rc) Review quickly please :) I'm planning to submit this in a day or two, unless someone objects until then. It's not the nicest design, I'll improve later, but right now it's a reasonable quick-fix I guess. What I dislike the most is the reset() and advance_tail() methods calling each other – in fact, they are different entry points to pretty much the same functionality. In advance_tail(), if the tail happens to reach head, we reset() the streams for additional benefits (e.g. the actual file offset is reset to 0). On the other hand, in reset() we're not allowed to retreat head, so it's effectively an advance_tail() to the new head; except for the lack of an assertion, the new tail is allowed to bypass the old head in this case and then head will be incremented too. Ideally, on top of that other linked bug, we'd merge these two actions into a single function.
Fixed. master: https://git.gnome.org/browse/vte/commit/?id=a235384 0-40: https://git.gnome.org/browse/vte/commit/?h=vte-0-40&id=5e41c83
I didn't follow the 100% details, but looks good. Thanks Egmont!