GNOME Bugzilla – Bug 777493
g_mkdtemp() not introspectable
Last modified: 2017-01-24 10:08:01 UTC
Using mozjs31-31.5.0-1.fc26.x86_64 from Fedora rawhide and gjs and gnome-shell from jhbuild, with the patch below applied on top of gnome-shell, I get a double-free when running gnome-shell-portal-helper (activated through "gapplication launch org.gnome.Shell.PortalHelper" after running the binary). The patch: diff --git a/js/portalHelper/main.js b/js/portalHelper/main.js index 3984642..dbf32cb 100644 --- a/js/portalHelper/main.js +++ b/js/portalHelper/main.js @@ -63,8 +63,14 @@ const PortalWindow = new Lang.Class({ this._doneCallback = doneCallback; this._lastRecheck = 0; this._recheckAtExit = false; + this._cacheDir = GLib.mkdtemp(GLib.get_tmp_dir() + "/XXXXXXXX"); - this._webView = new WebKit.WebView(); + print (this._cacheDir); + let dataManager = new WebKit.WebSite.data_manager({ base_data_directory: this._cacheDir, + base_cache_directory: this._cacheDir }); + let context = new WebKit.WebContext({ website_data_manager: dataManager }); + + this._webView = WebKit.WebView.new_with_context(context); this._webView.connect('decide-policy', Lang.bind(this, this._onDecidePolicy)); this._webView.load_uri(url); this._webView.connect('notify::title', Lang.bind(this, this._syncTitle)); The crash:
+ Trace 237072
This may be another instance of https://bugs.webkit.org/show_bug.cgi?id=116672, but I've recently gotten another report of rawhide GJS crashing, so I'll check this out before claiming it's WebKit's fault :-)
Here's a shorter reproducer: const GLib = imports.gi.GLib; GLib.mkdtemp("XXXXXX"); GJS is confused by the return value and the input argument sharing the same memory, which cannot be represented correctly with a GObject introspection annotation. I see there are these two closed bugs as well indicating that it is not intended to be fixed: https://bugzilla.gnome.org/show_bug.cgi?id=679351 https://bugzilla.gnome.org/show_bug.cgi?id=679362 I would suggest using GLib.Dir.make_tmp() instead. g_mkdtemp() and friends should be annotated (skip) in my opinion. So here's a GLib patch and I'll move the bug over to GLib.
Created attachment 343973 [details] [review] introspection: Skip g_mkdtemp() and friends Based on bugs [1] and [2], gobject-introspection does not handle this modify-input-and-return pattern correctly, and is not going to. Therefore, these functions should be skipped in introspection. [1] https://bugzilla.gnome.org/show_bug.cgi?id=679351 [2] https://bugzilla.gnome.org/show_bug.cgi?id=679362
(In reply to Philip Chimento from comment #2) > Here's a shorter reproducer: > > const GLib = imports.gi.GLib; > GLib.mkdtemp("XXXXXX"); > > GJS is confused by the return value and the input argument sharing the same > memory, which cannot be represented correctly with a GObject introspection > annotation. > > I see there are these two closed bugs as well indicating that it is not > intended to be fixed: > > https://bugzilla.gnome.org/show_bug.cgi?id=679351 > https://bugzilla.gnome.org/show_bug.cgi?id=679362 > > I would suggest using GLib.Dir.make_tmp() instead. > > g_mkdtemp() and friends should be annotated (skip) in my opinion. So here's > a GLib patch and I'll move the bug over to GLib. Would also be nice to mention g_dir_make_tmp() in the docs for the functions you're marking as skip. It would never have occurred to me to look for this, especially as g_dir_make_tmp() doesn't take a GDir...
I'm not entirely sure I agree with skipping the function - even though it is a pretty C-oriented API. The way `g_mkdtemp()` works is by replacing the six XXXXXX characters in the template buffer with something else; it does not allocate a new string and discard the old one. Using `inout` as the annotation should work. (In reply to Bastien Nocera from comment #4) Since you're already use `g_get_tmp_dir()` and GIO, you probably want to use `g_file_new_tmp()` instead: https://developer.gnome.org/gio/stable/GFile.html#g-file-new-tmp > Would also be nice to mention g_dir_make_tmp() in the docs for the functions > you're marking as skip. It would never have occurred to me to look for this, > especially as g_dir_make_tmp() doesn't take a GDir... This should happen regardless...
Created attachment 344012 [details] [review] introspection: Fix introspection for g_mkdtemp() and friends Mark g_mkdtemp(), g_mkstemp() and the _full() versions as having "inout" parameters, making them usable through introspection.
Created attachment 344013 [details] [review] gir: Update annotations from GLib git master
That's 2 patches for glib and gobject-introspection, respectively, but gjs still crashes. The valgrind errors make it look like a plain bug in gjs.
Created attachment 344014 [details] [review] gfileutils: Fix g_mkdtemp*() API docs Don't refer to g_mkdtemp() when documenting g_mkdtemp_full() and speaking about the function itself, and remove mention of flags in aforementioned g_mkdtemp_full(), as it doesn't have such an argument (but g_mkstemp_full() does).
Created attachment 344015 [details] [review] gfileutils: Mention g_dir_make_tmp() in g_mkdtemp*() docs
There's still no way using g-i annotation to indicate that (In reply to Emmanuele Bassi (:ebassi) from comment #5) > I'm not entirely sure I agree with skipping the function - even though it is > a pretty C-oriented API. > > The way `g_mkdtemp()` works is by replacing the six XXXXXX characters in the > template buffer with something else; it does not allocate a new string and > discard the old one. Using `inout` as the annotation should work. I'm not convinced yet. There's still no way to use g-i annotations to indicate that the (out) parameter and the return value point to the same memory. I think we at least need to have a (transfer none) annotation on the return value so that GJS/PyGObject are forced to copy it. Or is it even possible to (skip) the return value? Otherwise you can end up with two JS/Python strings that point to the same memory. (In JS at least this is possible to have, but the problem is there's no way for the binding to know about it.) However, I'm inclined to agree with Colin here [1] and say that these functions are not introspectable, since it's really a C convenience API and we already have good bindable alternatives. [1] https://bugzilla.gnome.org/show_bug.cgi?id=679351#c5
(In reply to Philip Chimento from comment #11) <snip> > Otherwise you can end up with two JS/Python strings that point to the same > memory. (In JS at least this is possible to have, but the problem is there's > no way for the binding to know about it.) Didn't think about that, so fair point on making those functions "(skip)".
Review of attachment 344014 [details] [review]: Ack.
Review of attachment 344015 [details] [review]: ::: glib/gfileutils.c @@ +1434,3 @@ * + * If you are going to be creating a temporary directory inside the + * preferred directory for temporary files, you might want to use ‘preferred system directory’ perhaps? Otherwise this raises the question of ‘preferred by whom’.
(In reply to Philip Withnall from comment #14) > Review of attachment 344015 [details] [review] [review]: > > ::: glib/gfileutils.c > @@ +1434,3 @@ > * > + * If you are going to be creating a temporary directory inside the > + * preferred directory for temporary files, you might want to use > > ‘preferred system directory’ perhaps? Otherwise this raises the question of > ‘preferred by whom’. It's the same expression used in the documentation for g_dir_make_tmp().
Comment on attachment 344014 [details] [review] gfileutils: Fix g_mkdtemp*() API docs Attachment 344014 [details] pushed as bc83612 - gfileutils: Fix g_mkdtemp*() API docs
(In reply to Bastien Nocera from comment #15) > (In reply to Philip Withnall from comment #14) > > Review of attachment 344015 [details] [review] [review] [review]: > > > > ::: glib/gfileutils.c > > @@ +1434,3 @@ > > * > > + * If you are going to be creating a temporary directory inside the > > + * preferred directory for temporary files, you might want to use > > > > ‘preferred system directory’ perhaps? Otherwise this raises the question of > > ‘preferred by whom’. > > It's the same expression used in the documentation for g_dir_make_tmp(). Yeah, I’m wondering that should be clarified too. I’m open to a second opinion. If we decide to ignore my review comment here, the patch is a_c-n as far as I’m concerned.
Created attachment 344101 [details] [review] gfileutils: Mention g_dir_make_tmp() in g_mkdtemp*() docs
Created attachment 344102 [details] [review] introspection: Skip g_mkdtemp() and friends Based on bugs [1] and [2], gobject-introspection does not handle the same string being 1) returned from an inout argument and 2) returned as the function's return value, and is not going to. Therefore, these functions should be skipped in introspection. [1] https://bugzilla.gnome.org/show_bug.cgi?id=679351 [2] https://bugzilla.gnome.org/show_bug.cgi?id=679362
Attachment 344101 [details] pushed as 810a6eb - gfileutils: Mention g_dir_make_tmp() in g_mkdtemp*() docs Attachment 344102 [details] pushed as 4fbcd18 - introspection: Skip g_mkdtemp() and friends