Opened 3 years ago

Closed 3 years ago

#24522 closed defect (duplicate)

Pynac should check for errors when calling PyObject_RichCompareBool()

Reported by: embray Owned by: jdemeyer
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: packages: standard Keywords: pynac
Cc: rws Merged in:
Authors: Reviewers:
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by 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: https://github.com/pynac/pynac/pull/297

Change History (9)

comment:1 Changed 3 years ago by embray

  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 3 years ago by embray

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.

comment:3 Changed 3 years ago by jdemeyer

  • Cc rws added
  • Component changed from python3 to packages: standard
  • Description modified (diff)
  • Owner changed from (none) to jdemeyer
  • Summary changed from py3: bug in pynac numeric.cpp with PyObject_RichCompareBool to Pynac should check for errors when calling PyObject_RichCompareBool()

comment:4 Changed 3 years ago by embray

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

comment:5 Changed 3 years ago by embray

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:6 Changed 3 years ago by rws

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.

comment:7 Changed 3 years ago by embray

  • Priority changed from major to minor

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.

comment:8 Changed 3 years ago by chapoton

  • Milestone changed from sage-8.2 to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

Can we close this now ?

comment:9 Changed 3 years ago by jdemeyer

  • Resolution set to duplicate
  • Status changed from needs_review to closed
Note: See TracTickets for help on using tickets.