GNOME Bugzilla – Bug 577999
converting a negative long Python value to a GUINT64 GValue doesn't error out as it should
Last modified: 2009-04-05 14:40:37 UTC
Consider this example in the Python interpreter: (gdb) r Starting program: /usr/bin/python [Thread debugging using libthread_db enabled] Python 2.5.1 (r251:54863, Jun 15 2008, 18:24:51) [GCC 4.3.0 20080428 (Red Hat 4.3.0-8)] on linux2 Type "help", "copyright", "credits" or "license" for more information. [New Thread 0xb80856c0 (LWP 28074)] >>> import gst Detaching after fork from child process 28077. >>> e = gst.element_factory_make('gnlsource') >>> e.props.start = -1L >>> import gobject Traceback (most recent call last):
+ Trace 214192
The error happens because the property setting code doesn't properly check for PyErr_Occurred anymore the way it did before, and the way it does for INT64. This was changed in revision 447 by gcj, with commit message "guint64 property fix": case G_TYPE_UINT64: - g_value_set_uint64(value, PyLong_AsUnsignedLongLong(obj)); - if (PyErr_Occurred()) { - g_value_unset(value); - PyErr_Clear(); - return -1; - } + if (PyInt_Check(obj)) + g_value_set_uint64(value, PyInt_AsLong(obj)); + else if (PyLong_Check(obj)) + g_value_set_uint64(value, PyLong_AsUnsignedLongLong(obj)); + else + return -1; This change should also check for PyErr_Occurred in the second case, because not every PyLong is an UnsignedLongLong.
Hey, I'm gjc, not gcj :P First a comment johan that committed this: /* Compilation on Python 2.x */ #if PY_VERSION_HEX < 0x03000000 [...] #define _PyLong_Check PyInt_Check I think this makes the following code _harder_ to read. Bad idea IMHO. case G_TYPE_UINT64: if (_PyLong_Check(obj)) g_value_set_uint64(value, _PyLong_AsLong(obj)); else if (PyLong_Check(obj)) g_value_set_uint64(value, PyLong_AsUnsignedLongLong(obj)); else return -1; break; Before the python 3.x compatibility layer, the code used to read: case G_TYPE_UINT64: if (PyInt_Check(obj)) g_value_set_uint64(value, PyInt_AsLong(obj)); else if (PyLong_Check(obj)) g_value_set_uint64(value, PyLong_AsUnsignedLongLong(obj)); else return -1; break; Which is broken but at least I don't have to decipher what _PyLong_Check means, and it makes sense (now it looks like it calls PyLong_Check twice; why? until you read that _PyLong_Check means PyInt_Check, it doesn't make sense at all). The only problem here seems to be that PyLong_AsUnsignedLongLong does not check for negative values, as it should. I notice that they will fix it in Python 2.7, but we definitely want to fix it for older Pythons.
Sorry, I misread the documentation, that says, """Changed in version 2.7: A negative pylong now raises OverflowError, not TypeError.""" So it already did raise an exception, just the wrong one.
Created attachment 132116 [details] [review] proposed patch
Thanks for the quick reply! Does pygobject have a unit test setup ? I think these kinds of bugs are very subtle and very hard, so it would make sense to unit test them. The way you change the code in your patch it's not very obvious that all the cases are correctly handled.
See tests/test_properties.py
Created attachment 132126 [details] [review] fixed patch plus unit test