GNOME Bugzilla – Bug 740823
[PATCH] Load C extensions with G_MODULE_BIND_LOCAL
Last modified: 2014-12-18 14:15:26 UTC
C extensions can bring arbitrary symbols into the global symbol table which is then shared with all other extensions and the main binary. This is a recipe for symbol clashes which are hard to debug. G_MODULE_BIND_LOCAL provides proper isolation and makes the applications more robust. Note: This has been the libpeas default until it was determined that it breaks the python loader. Therefore this change only affects user extensions, and not loaders. Considering that the loaders are part of libpeas and well tested and trusted this is not much of a problem.
Created attachment 291683 [details] [review] Test case, fails initially
Created attachment 291684 [details] [review] actual change
Created attachment 291685 [details] [review] actual change;, v2 - corrected @since tag
Review of attachment 291685 [details] [review]: ::: libpeas/peas-engine.c @@ +645,3 @@ + + /* Bind loaders gloally. Binding locally broke the Python plugin loader. */ + module = peas_object_module_new (module_name, module_dir, TRUE, FALSE); Extra newline ::: libpeas/peas-object-module.c @@ +276,3 @@ + /** + * PeasObjectMpodule:local-linkage typo, should be PeasObjectModule @@ +279,3 @@ + * + * This property indicates whether the module was loaded with local linkage, i.e. if + * the @G_MODULE_BIND_LOCAL was specified at loading. s/at loading// @@ +311,3 @@ const gchar *path, + gboolean resident, + gboolean local_linkage) This breaks API and ABI, needs an alternative
Thanks for your review. I was under the impression that the PeasObjectModule is private to libpeas. Its gtk-doc comment has a (skip) annotation, and it does not appear in the reference manual[1]. The only public functions of PeasObjectModule are _register_extension_type() and _register_extension_factory(). So how can this constitute and API/ABI break? [1] https://developer.gnome.org/libpeas/stable/PeasObjectModule.html
Correction: "I was under the impression that the PeasObjectModule constructor is private to libpeas."
Because the header is still installed and because it is not prefixed with an '_' thus it is part of the library. This is mostly for historic reasons where the C plugin loader was not included in the libpeas library itself and was instead loaded like the other plugin loaders. gnome/libpeas (j)$ nm -g libpeas/.libs/libpeas-1.0.so | grep 'peas_object_module_' 000000000000d278 T peas_object_module_create_object 000000000000d5ce T peas_object_module_get_library 000000000000d530 T peas_object_module_get_module_name 000000000000d492 T peas_object_module_get_path 000000000000c99a T peas_object_module_get_type 000000000000d209 T peas_object_module_new 000000000000d3a2 T peas_object_module_provides_object 000000000000d66b T peas_object_module_register_extension_factory 000000000000d887 T peas_object_module_register_extension_type
But it is not formally part of the API, and not document. So any client code using it is invalid. On the other hand, if you consider it part of the API by preventing any changes to it, then it should documented as such.
The point is that people could be using it as the header is installed with the symbol, for some symbols in the library the header is not installed at all, i.e. peas_engine_shutdown(). For those symbols I would be fine with removing/modifying them, but this one cannot be done that way. Anyways, for this to go in another name will need to be used for we might just use g_object_new() directly, doesn't really hurt us much.
Added a new function peas_object_module_new_full() is prefered.
And if it is extended again it needs another new function, peas_object_module_new_fuller()? Or should it be private from the outset?
Appending _full() is the general convention, it could be made private but at this point it doesn't matter that much and probably isn't worth the headache.
Alright, but what if we need to extend it another time?
Created attachment 292135 [details] [review] actual change; v3 - added peas_object_module_new_full() Here's a version with peas_object_module_new_full() as requested. The old peas_object_module_new() does not set the construct property, and thus will default to FALSE, as per status quo. I changed the libpeas callers to the _full functions, though, to make setting the prop explicit. I also thought about requiring callers of peas_object_module_new_full() to pass in the properties themselves, similar to g_object_new() (except we dictate the GType). Then we could add more props without needing a another _new() variant, provided we assign compatible defaults. What do you think? I don't feel strong about this, though. I also fixed the other remarks. From my side this can go in.
Created attachment 292982 [details] [review] Load C plugins with local linkage v4 C extensions can bring arbitrary symbols into the global symbol table which is then shared with all other extensions and the main binary. This is a recipe for symbol clashes which are hard to debug. Using G_MODULE_BIND_LOCAL provides proper isolation and makes applications more robust. Based on a patch by Thomas Martitz https://bugzilla.gnome.org/show_bug.cgi?id=740823
Created attachment 292983 [details] [review] Test that C plugins have local linkage v2 Otherwise two symbols from different plugins could share the same global reference. Based on a patch by Thomas Martitz https://bugzilla.gnome.org/show_bug.cgi?id=740823
Review of attachment 292982 [details] [review]: Looks good.
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.