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 728051 - segv in vte_terminal_get_rgb_from_index
segv in vte_terminal_get_rgb_from_index
Status: RESOLVED FIXED
Product: vte
Classification: Core
Component: general
0.36.x
Other Linux
: Normal critical
: ---
Assigned To: VTE Maintainers
VTE Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-04-11 17:27 UTC by Christian Persch
Modified: 2014-04-26 15:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
maybe fix? (970 bytes, patch)
2014-04-11 21:47 UTC, Egmont Koblinger
none Details | Review
maybe fix? v2 (2.90 KB, patch)
2014-04-11 22:28 UTC, Egmont Koblinger
committed Details | Review

Description Christian Persch 2014-04-11 17:27:56 UTC
From https://bugzilla.redhat.com/show_bug.cgi?id=1084652 [https://retrace.fedoraproject.org/faf/reports/394777/]:

Thread 1 (Thread 0x7fb92ff9fac0 (LWP 4623))

  • #0 memcpy
    at /usr/include/bits/string3.h line 51
  • #1 vte_terminal_get_rgb_from_index
    at vte.c line 6321
  • #2 vte_terminal_get_text_range_maybe_wrapped
    at vte.c line 6428

Looks vte_terminal_get_color returned NULL here.

I noticed that there are more places in our code where the return value isn't checked and the colour from the out value used nevertheless.
Comment 1 Christian Persch 2014-04-11 17:29:50 UTC
s/and the colour from the out value used nevertheless//
Comment 2 Egmont Koblinger 2014-04-11 21:46:46 UTC
What should happen:

Three special colors (cursor bg, highlight bg/fg) can be unset, so it's okay for vte_terminal_get_color to return NULL on these, but only on these. It should be okay to omit the null-check if the index is not one of these special ones.

The init code should set the palette accordingly, and the API shouldn't allow to unset any color except those three special ones.

A character cell should never have any of these special colors. The current crash occurs in vte_terminal_get_text_range_maybe_wrapped() where we call vte_terminal_get_rgb_from_index() on pcell->attr.fore/back which should correspond to a physical cell.

My first (and currently only) theory on what's going wrong:

The palette is initialized when the widget is realized, not when initialized. Is it possible that VteTerminal is initialized but not yet realized, and then someone calls vte_terminal_get_rgb_from_index()?
Comment 3 Egmont Koblinger 2014-04-11 21:47:43 UTC
Created attachment 274136 [details] [review]
maybe fix?
Comment 4 Egmont Koblinger 2014-04-11 22:28:27 UTC
Created attachment 274138 [details] [review]
maybe fix? v2

If the previous patch works, here's one with further cleanup (some code simplification and comment fixes).
Comment 5 Egmont Koblinger 2014-04-14 15:33:22 UTC
Comment on attachment 274138 [details] [review]
maybe fix? v2

"Maybe fix" submitted. Let's watch downstream bug to see if it fixes the issue.
Comment 6 İsmail Dönmez 2014-04-24 10:32:14 UTC
I tested the patch on openSUSE 13.1 with GNOME 3.12 and it fixes the qemu crash for me. Thanks!
Comment 7 Egmont Koblinger 2014-04-24 11:02:22 UTC
İsmail, thanks a lot for your feedback, it's great to hear it :)

ChPe, given the severity, could you please do a vte-0.36.1 release?