After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 748484 - crash in _vte_boa_reset
crash in _vte_boa_reset
Status: RESOLVED FIXED
Product: vte
Classification: Core
Component: general
0.40.x
Other Linux
: Normal critical
: ---
Assigned To: VTE Maintainers
VTE Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-04-26 14:18 UTC by Christian Persch
Modified: 2015-04-28 21:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
v0 (wip) (2.49 KB, patch)
2015-04-26 17:34 UTC, Egmont Koblinger
none Details | Review
v1 (wip) (3.78 KB, patch)
2015-04-26 18:04 UTC, Egmont Koblinger
none Details | Review
v2 (rc) (4.50 KB, patch)
2015-04-27 09:13 UTC, Egmont Koblinger
none Details | Review

Description Christian Persch 2015-04-26 14:18:02 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
Comment 1 Egmont Koblinger 2015-04-26 15:05:21 UTC
Oops.

assertion failed (offset >= boa->head): (0 >= 65512)

Will take a look.
Comment 2 Egmont Koblinger 2015-04-26 17:34:21 UTC
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...
Comment 3 Egmont Koblinger 2015-04-26 18:04:33 UTC
Created attachment 302391 [details] [review]
v1 (wip)

This is not the cleanest part of vte... I'll see if I can simplify.
Comment 4 Egmont Koblinger 2015-04-26 18:44:56 UTC
(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.
Comment 5 Behdad Esfahbod 2015-04-26 20:18:11 UTC
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 6 Behdad Esfahbod 2015-04-26 20:36:17 UTC
Comment was meant for bug 748492.
Comment 7 Egmont Koblinger 2015-04-27 09:13:08 UTC
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.
Comment 9 Behdad Esfahbod 2015-04-28 21:58:40 UTC
I didn't follow the 100% details, but looks good.  Thanks Egmont!