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 795826 - Rare hyperlink related crash
Rare hyperlink related crash
Status: RESOLVED FIXED
Product: vte
Classification: Core
Component: general
0.52.x
Other Linux
: Normal critical
: ---
Assigned To: VTE Maintainers
VTE Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-05-05 08:40 UTC by gnomebugs
Modified: 2018-05-21 20:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
backtrace and registers (2.03 KB, text/plain)
2018-05-05 08:40 UTC, gnomebugs
  Details
reproduce crash in strchr (196.46 KB, video/mp4)
2018-05-05 10:36 UTC, gnomebugs
  Details
variant 2: reproduce crash with failed assertion (120.74 KB, video/mp4)
2018-05-05 10:37 UTC, gnomebugs
  Details
variant 2: backtrace (1.76 KB, text/plain)
2018-05-05 10:37 UTC, gnomebugs
  Details
Fix (498 bytes, patch)
2018-05-12 18:32 UTC, Egmont Koblinger
none Details | Review
Fix, v2 (477 bytes, patch)
2018-05-12 19:07 UTC, Egmont Koblinger
committed Details | Review
Fix for 0.52 and 0.50 (540 bytes, patch)
2018-05-12 19:28 UTC, Egmont Koblinger
committed Details | Review

Description gnomebugs 2018-05-05 08:40:49 UTC
Created attachment 371690 [details]
backtrace and registers

Hi,

I'm using VTE 0.52.1 on Arch Linux. I have a little program that uses VTE and I saw some crashes when hovering over explicit hyperlinks:

https://github.com/vain/xiate/issues/13

I tried to gather as much information as possible, but my analysis is not complete and I can't provide a patch, yet. :( Sorry for that.

It appears that VTE is calling `strchr(NULL, ';')` in line 5660 of vte.cc on the vte-0-52 branch:

https://github.com/GNOME/vte/blob/d00c15afcd4f50cc57d6bf3bcc87bd6cedf25529/src/vte.cc#L5660

At least, this is what I think gdb is trying to tell me. For full "analysis" see my GitHub comment, but I attached the relevant details here. I does seem somewhat possible, since the `if` only guards for `m_hyperlink_hover_idx != 0`, not for `m_hyperlink_hover_uri`.

As mentioned on GitHub, I was using a tool that prints a lot of those new explicit hyperlinks. It does so every few seconds, so the terminal window is often scrolling automatically. I was then using my mouse to hover over such a link, which very rarely results in a crash. I cannot reproduce this reliably.

I don't think it's relevant, but a hyperlink created by the tool looks like this (so it's a link showing two unicode block characters surrounded by a foreground color change):

printf '\e[38;5;250m\e]8;;10.1.45.12 (0.7 ms this, 0.7 ms avg)\a█▉\e]8;;\a\e[0m\n'

Can you work with this? Let me know if you need anything else.

Thank you!
Comment 1 Egmont Koblinger 2018-05-05 09:05:03 UTC
Thanks, I'll try to see if I can find the culprit based on this.

Do you happen to know: Was the bug present in vte-0.50.1 or 0.50.2? There were some changes shortly before the release of 0.52, relevant to how regex matches and hyperlinks are updated when the mouse is moved – the scenario that triggers the crash for you. I'm wondering if the bug might have sneaked in then.

(Irrelevant: So you're not using this feature to have actual hyperlinks, but some arbitrary tooltip text... interesting use case that didn't occur to me :))
Comment 2 gnomebugs 2018-05-05 10:36:08 UTC
Thanks for the quick reply!


I finally found a way to reproduce the crashes. Turns out, there are two
variants. Let's focus on the crash in strchr() first.

Steps to reproduce:

    - Run a hyperlink-enabled VTE terminal under X11. It doesn't have to
      be my program (xiate), it happens in other terminals as well, such
      as guake.
    - Slightly increase the size of the X11 window, so there's a small
      gap below the terminal widget.
    - Run this command:

        while sleep 0.1; do printf '\n'; for i in {1..15}; do printf '\033]8;;%s\a%s\033]8;;\a' "$(date)" welt; done; done

    - Wait for the output to reach the bottom of the terminal.
    - Move the mouse to the last row at the bottom and then move it over
      the small gap between terminal widget and window edge.
    - The terminal will nearly always crash.

I have a video of the process (`crash-in-strchr.mp4`), which I'll attach
shortly. (In the video, there are some leading spaces and color codes,
but they're irrelevant.)

Also happens with Openbox as window manager.

-----

The other crash variant happens when there's just one link per line.
That would be `VARIANT-2-crash-with-assertion-failed.mp4`. It results in
a different stack trace (`VARIANT-2-backtrace-assertion-failed.txt`).
I'd be happy to open another bug report for that one, but since it's
very similar, maybe the two variants are connected?

-----

Now that I can reproduce the crash, I rebuilt 0.50.1 and 0.50.2: Both
crash variants are present in both of the older versions.

-----

(Yes, I'm totally abusing hyperlinks to actually get tooltips. :))
Comment 3 gnomebugs 2018-05-05 10:36:47 UTC
Created attachment 371703 [details]
reproduce crash in strchr
Comment 4 gnomebugs 2018-05-05 10:37:18 UTC
Created attachment 371704 [details]
variant 2: reproduce crash with failed assertion
Comment 5 gnomebugs 2018-05-05 10:37:39 UTC
Created attachment 371705 [details]
variant 2: backtrace
Comment 6 Egmont Koblinger 2018-05-08 07:49:24 UTC
Thanks for the reproducible test case! Please be patient for a couple of days; I'll work on it as soon as I have a free evening with a relatively fresh mind :)
Comment 7 Egmont Koblinger 2018-05-12 18:32:51 UTC
Created attachment 371967 [details] [review]
Fix

Could you please test this patch?

As also seen in the first video, sometimes moving the mouse to the "extra padding" at the bottom does not cause immediate segfault, but causes the previous line's hyperlink one to the right (that is, not exactly above the mouse) getting underlined.

In vte.cc hyperlink_hilite_update(), _vte_ring_get_hyperlink_at_position sometimes returns an erroneous value, which triggers the segfault a few code lines later (if that erroneous value doesn't happen to be a valid hyperlink at that point; otherwise a faulty underlining happens).

In ring.cc, _vte_ring_get_hyperlink_at_position() is not prepared to be passed a vertical position that's beyond (below) the end of the ring. I didn't track down how it exactly goes haywire, but I guess it gets fooled by the ring buffer's modulo arithmetics, and sometimes refers to the row that's just about to get frozen (written from the ring to the stream; this assumption is backed up by the observation that the script needs to be run for a short while before the segfault can be reproduced), or sometimes just accesses out-of-bounds memory address, or whatever, doesn't really matter.

So the relevant bit from the patch is the addition of the "next()" guard.

The "m_start" guard should be irrelevant, since the function shouldn't be able to receive a smaller value; still, I think it's more defensive against further changes, e.g. a model/view split. The "col < 0" is purely cosmetical (it's a signed int) to make it look more similar to the others.
Comment 8 Egmont Koblinger 2018-05-12 19:07:00 UTC
Created attachment 371968 [details] [review]
Fix, v2

Let's use existing handy convenience methods for even cleaner code :)
Comment 9 Egmont Koblinger 2018-05-12 19:28:15 UTC
Created attachment 371969 [details] [review]
Fix for 0.52 and 0.50

Here's the fix for 0-52 and 0-50.

(Master has been C++ified recently, so it's pretty different.)

(0-48 didn't have hyperlink support yet, so it's unaffected.)
Comment 10 gnomebugs 2018-05-13 05:20:09 UTC
Looks good. I no longer see crashes with 0-50, 0-52, or current master (fd1956fb).

Thank you! :)
Comment 11 Egmont Koblinger 2018-05-13 08:45:59 UTC
You're welcome :)

Christian, take a look please, and then I suggest we roll out stable releases along with the scheduled unstable ones Monday week, if that sounds good to you too.
Comment 12 Egmont Koblinger 2018-05-13 13:25:19 UTC
Thinking a bit more about _how_ it exactly goes haywire (before applying the patch):

The overflow happens before the modulo arithmetics of the ring. So I think there's no out-of-bounds data access, or access to uninitialized memory. It just accesses the 32nd (64th, etc.) previous row. That's why hovering over the bottom extra padding didn't cause problems when no hyperlinks were involved at all: reading from the faulty area still properly reported that it's not a hyperlink.

It is, however, probably possible to crash VTE even when only a few hyperlinks were present that have all been scrolled out by a bit when moving around the mouse and triggering the crash. (I haven't tried.)
Comment 13 Egmont Koblinger 2018-05-21 20:52:49 UTC
VTE 0.52.2 and 0.50.4 released with this fix.

Notified:
 - Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=1580586
 - Ubuntu: https://bugs.launchpad.net/ubuntu/+source/vte2.91/+bug/1772506

Bumping the severity retroactively, after all, it's a crash :)

Closing. Thanks again for the report!