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 751956 - Crash in signal handler with GError parameter
Crash in signal handler with GError parameter
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: general
Git master
Other Linux
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
: 751941 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-07-04 20:21 UTC by Christoph Reiter (lazka)
Modified: 2016-03-02 10:05 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
crashing example (728 bytes, text/plain)
2015-07-04 20:21 UTC, Christoph Reiter (lazka) 		  Details
GVariant: Don't use pyg_boxed_new as GVariant isn't a PyGBoxed but a PyGIStruct. (2.84 KB, patch)
2015-07-04 20:22 UTC, Christoph Reiter (lazka) 		committed Details | Review
Some error handling/reporting fixes. (3.42 KB, patch)
2015-07-04 20:23 UTC, Christoph Reiter (lazka) 		committed Details | Review

Description Christoph Reiter (lazka) 2015-07-04 20:21:09 UTC 
Created attachment 306833 [details]
crashing example

TingPing reported a crasher on IRC with code to reproduce. Example attached.

The problem is that GUPnPIgd.SimpleIgd::error-mapping-port crashes.

The cause is that the Python GError class gets instantiated as a GBoxed in the marshalling code and crashes when deallocated. I guess the fix is to add a special case for GError values in there.

The following two patches improve the error handling so we get an error message instead of a crash in the future.
Comment 1 Christoph Reiter (lazka) 2015-07-04 20:22:28 UTC 
Created attachment 306834 [details] [review]
GVariant: Don't use pyg_boxed_new as GVariant isn't a  PyGBoxed but a PyGIStruct.

This only worked because they share the same struct layout.
This adds a new constructor for creating a new PyGIStruct instance from GType.

----

Note: this is needed first as the next patch exposed this error.
Comment 2 Christoph Reiter (lazka) 2015-07-04 20:23:29 UTC 
Created attachment 306835 [details] [review]
Some error handling/reporting fixes.

* Check in pyg_boxed_new() if the passed type is an actual subclass
* Don't replace existing exceptions in pyg_value_as_pyobject()
* Print an error in pyg_closure_marshal() in case marshalling
  an argument failed.
Comment 3 Jens Georg 2015-07-05 13:54:59 UTC 
*** Bug 751941 has been marked as a duplicate of this bug. ***
Comment 4 Simon Feltman 2016-02-28 20:50:43 UTC 
Review of attachment 306834 [details] [review]:

Seems fine to me. I've verified this new function is exercised multiple times by the test suite.
Comment 5 Simon Feltman 2016-02-28 20:53:59 UTC 
Review of attachment 306835 [details] [review]:

Overall seems fine. Are there any signals in GLib, GObject, Regress, or GIMarshallingTests similar to GUPnPIgd.SimpleIgd::error-mapping-port which we may use to test these errors?
Comment 6 Simon Feltman 2016-02-28 21:48:11 UTC 
Review of attachment 306835 [details] [review]:

I've verified that at least the change to pygi-value.c is exercised by: test_generictreemodel.TestReturnsAfterError
Comment 7 Christoph Reiter (lazka) 2016-02-29 13:58:47 UTC 
Thanks, pushed to master.

I can't reproduce the crash/error case anymore :/
Comment 8 Simon Feltman 2016-03-02 08:08:00 UTC 
Is this ticket ok to close?
Comment 9 Christoph Reiter (lazka) 2016-03-02 10:05:04 UTC 
I guess so. At least we'll get an error in case this come sup again.