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 730407 - matcher free_params code smell
matcher free_params code smell
Status: RESOLVED FIXED
Product: vte
Classification: Core
Component: general
0.37.x
Other Linux
: Normal normal
: ---
Assigned To: VTE Maintainers
VTE Maintainers
[fixed:vteparser]
Depends on: vteparser
Blocks:
 
 
Reported: 2014-05-19 22:03 UTC by Egmont Koblinger
Modified: 2018-03-27 17:48 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Egmont Koblinger 2014-05-19 22:03:42 UTC
_vte_matcher objects are stored in a GCache keyed by the terminfo data, or as a singleton if the patch from bug 728900 comment 13 is already applied. This is so that the same matcher is shared among many VteTerminal objects.

The object, however, contains a _vte_matcher_free_params_array() method which alters the free_params field. It kinda immediately frees some pointers (but what does it have to do with the matcher?), and/or registers them to be freed later? I don't understand that code at all.

vte.c has this in vte_terminal_process_incoming():

next_match:
                if (G_LIKELY(params != NULL)) {
                        /* Free any parameters we don't care about any more. */
                        _vte_matcher_free_params_array(terminal->pvt->matcher,
                                        params);
                }

which looks like it's manipulating the matcher's free_params field, as if it was its own, rather than shared across multiple terminals.

I don't understand what's going on, but it's really fishy.
Comment 1 Behdad Esfahbod 2014-05-22 00:04:32 UTC
From what I understand, matcher->free_params is used to cache one GValueArray to reduce allocation/free overhead!
Comment 2 Egmont Koblinger 2014-05-22 00:39:12 UTC
The main question is whether it's multithread-safe. (Multiple VteTerminals from different threads accessing their common matcher->free_params concurrently.) I'm not certain at all.
Comment 3 Behdad Esfahbod 2014-05-22 00:56:02 UTC
It's not.

I'm working on hugely simplifying this piece of code.  Only if I could actually get it compiling...
Comment 4 Egmont Koblinger 2014-05-22 01:22:04 UTC
Nice to see you back on the project :)
Comment 5 Behdad Esfahbod 2014-05-22 01:50:32 UTC
Thanks for turning its head back towards sanity!
Comment 6 Christian Persch 2018-03-27 17:48:38 UTC
Fixed on master (by dropping this code).