Pynac should check for errors when calling PyObject_RichCompareBool()

[embray]

There are a number of methods in pynac's ginac/numeric.cpp module that don't check for error results from PyObject_RichCompareBool (which returns -1 on error).

This leads to unhandled exceptions that, on Python 2, were somewhat by luck being cleared via PyObject_Clear() later in the call stack.

On Python 3, the interpreter asserts PyErr_Occurred in more places and we don't get so lucky.

Upstream PR: pynac/pynac#297

Upstream: Fixed upstream, in a later stable release.

CC: @rwst

Component: packages: standard

Keywords: pynac

Issue created by migration from https://trac.sagemath.org/ticket/24522

[embray]

comment:2

Yes, this still looks like what I described. On Python 3 the exception leaks out, and slightly later on a call to PyObject_IsInstance in turn calls the type's __instancecheck__ which goes through _PyObject_FastCallDict which in turn asserts PyErr_Occurred.

On Python 3 there is no _PyObject_FastCallDict, no assertion checks on PyErr_Occurred in the relevant code path, and eventually that error gets cleared.

[jdemeyer]

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
-There are a number of methods in pynac's numeric module that don't check for error results from `PyObject_RichCompareBool`.  This leads to unhandled exceptions that, on Python 2, were somewhat by luck being cleared via `PyObject_Clear()` later in the call stack.  Maybe this was somewhat by design but it doesn't feel that way.
+There are a number of methods in pynac's `ginac/numeric.cpp` module that don't check for error results from `PyObject_RichCompareBool` (which returns -1 on error).
 
-On Python 3 we're less lucky--the interpreter asserts `PyErr_Occurred` in more places and we don't get so lucky.  That, at least, seems to be the source of the difference.  I need to investigate a little more though.
+This leads to unhandled exceptions that, on Python 2, were somewhat by luck being cleared via `PyObject_Clear()` later in the call stack.
+
+On Python 3, the interpreter asserts `PyErr_Occurred` in more places and we don't get so lucky.

[embray]

comment:4

I have an upstream fix for this ready--just need to make a PR...

[embray]

Upstream: Reported upstream. No feedback yet.

[embray]

Description changed:

--- 
+++ 
@@ -3,3 +3,5 @@
 This leads to unhandled exceptions that, on Python 2, were somewhat by luck being cleared via `PyObject_Clear()` later in the call stack.
 
 On Python 3, the interpreter asserts `PyErr_Occurred` in more places and we don't get so lucky.
+
+**Upstream PR:** https://github.com/pynac/pynac/pull/297

[rwst]

Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.

[embray]

comment:7

Thanks! This is low priority for now, as it only affects a few things on Python 3. We can close this along with the next update to pynac in Sage.