Opened 6 years ago

Closed 4 years ago

#14211 closed defect (fixed)

Crash due to missing Cython exception handling for relational_to_bool()

Reported by: mjo Owned by: burcin
Priority: critical Milestone: sage-duplicate/invalid/wontfix
Component: symbolics Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by rws)

sage: f(x)=matrix()                           
sage: bool(f(x)==f(x))

terminate called after throwing an instance of 'std::runtime_error'
  what():  Number_T::hash() python function (__hash__) raised exception
------------------------------------------------------------------------
Unhandled SIGABRT: An abort() occurred in Sage.
This probably occurred because a *compiled* component of Sage has a bug
in it and is not properly wrapped with sig_on(), sig_off().
Sage will now terminate.
------------------------------------------------------------------------

Change History (27)

comment:1 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:2 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:3 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:4 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:5 Changed 4 years ago by rws

  • Priority changed from major to critical

comment:6 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-6.4 to sage-6.5

comment:7 Changed 4 years ago by rws

  • Milestone changed from sage-6.5 to sage-6.7
  • Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.

comment:8 follow-up: Changed 4 years ago by rws

  • Description modified (diff)

Minimal case given. In Sage hash(matrix()) will not allow hashing probably because PyObject_Hash will fail and to prevent that. The fail happens in Pynac nevertheless because the check is avoided due to function expansion and there leads to the crash.

One part of the fix is preventing Sage from crashing. Surrounding the call to Pynac in Expression::__nonzero__ with guards:

            sig_on()
            pynac_result = relational_to_bool(self._gobj)
            sig_off()

leads to:

sage: bool(f(x)==f(x))
terminate called after throwing an instance of 'std::runtime_error'
  what():  Number_T::hash() python function (__hash__) raised exception
---------------------------------------------------------------------------
TypeError: mutable matrices are unhashable

which is the same you get with hash(matrix()), so quite sensible. What more? Why are matrices generated by vector*vector (the original case) or those in a function definition (like above) mutable at all?

Last edited 4 years ago by rws (previous) (diff)

comment:9 Changed 4 years ago by rws

  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Reported upstream. Developers deny it's a bug.
  • Summary changed from Crash in GiNaC::Number_T::hash() to Crash due to missing sig_on in Expresion.__nonzero__

comment:10 Changed 4 years ago by rws

  • Summary changed from Crash due to missing sig_on in Expresion.__nonzero__ to Crash due to missing sig_on in Expression.__nonzero__

comment:11 in reply to: ↑ 8 ; follow-ups: Changed 4 years ago by nbruin

Replying to rws:

which is the same you get with hash(matrix()), so quite sensible. What more? Why are matrices generated by vector*vector (the original case) or those in a function definition (like above) mutable at all?

Because the default for matrix creation is to create a mutable matrix. That's because a mutable matrix can be turned into an immutable one, but not the other way around (that requires copying the matrix to a new, mutable, one). That default is probably inconvenient for symbolics.

Are you sure this is the sole problem, though? If we try this:

def imm_mt(*args,**kwargs):
    M=matrix(*args,**kwargs)
    M.set_immutable()
    return M
f(x)=imm_mt()

then we get

sage: hash(f(x))
0

but bool(f(x)==f(x)) still crashes. is it trying to see if f(x)*f(x)^(-1) is one?

comment:12 in reply to: ↑ 11 Changed 4 years ago by rws

Replying to nbruin:

Are you sure this is the sole problem, though?

I have reopened the Pynac issue. The crash is in exactly the same call tree. It happens while trying to multiply the rhs with -1 to get lhs-rhs. So:

sage: f(x)=matrix()
sage: bool(f(x)==1)
True
sage: bool(1==f(x))
<crash>

Also no, I don't think it's the sole problem. Either Sage or Pynac will have to check somewhere before evaluating that all mutable Python objects of the expression are switched to immutable (or make immutable copies).

EDIT: But that would be a different ticket: #18360

Last edited 4 years ago by rws (previous) (diff)

comment:14 Changed 4 years ago by jdemeyer

  • Summary changed from Crash due to missing sig_on in Expression.__nonzero__ to Crash due to missing Cython exception handling for relational_to_bool()

comment:15 Changed 4 years ago by jdemeyer

  • Report Upstream changed from Reported upstream. Developers deny it's a bug. to N/A

comment:16 Changed 4 years ago by rws

  • Branch set to u/rws/crash_due_to_missing_cython_exception_handling_for_relational_to_bool__

comment:17 in reply to: ↑ 11 Changed 4 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to b9698e065034425ff9d563b66a59616301288958
  • Status changed from new to needs_review

Replying to nbruin:

If we try this:

def imm_mt(*args,**kwargs):
    M=matrix(*args,**kwargs)
    M.set_immutable()
    return M
f(x)=imm_mt()

then we get

sage: hash(f(x))
0

but bool(f(x)==f(x)) still crashes.

My best guess at the moment is that somewhere in between a copy is made which, as you suggested, is then mutable again. This can only be fixed by a dedicated ticket such as #18360.

As to this code, the reviewer should check doubly if what I did here was right. There was not a single example in Sage with a special function catching exceptions from which I could have learned.


New commits:

b9698e014211: catch exceptions from Expression.__nonzero__

comment:18 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Obviously, a doctest is needed.

Besides that, it makes no sense to add an additional layer _nonzero_. The exception should be handled in the pynac interface, i.e. the relational_to_bool() function.

comment:19 Changed 4 years ago by rws

  • Branch changed from u/rws/crash_due_to_missing_cython_exception_handling_for_relational_to_bool__ to u/rws/14211

comment:20 follow-up: Changed 4 years ago by rws

  • Commit changed from b9698e065034425ff9d563b66a59616301288958 to e1374da2834bca4788084c91972e8c8c714e3b30
  • Status changed from needs_work to needs_review

Interesting. The C++ layer outputs an error message, and the Python layer deals with the error too, because presumably the error from calling PyHash from the C++ layer is still pending.


New commits:

e1374da14211: catch Pynac exceptions

comment:21 in reply to: ↑ 20 Changed 4 years ago by jdemeyer

Replying to rws:

Interesting. The C++ layer outputs an error message, and the Python layer deals with the error too, because presumably the error from calling PyHash from the C++ layer is still pending.

Well, behaviour shouldn't depend on what "presumably" happens (does the traceback look sensible? If not, you're certainly doing it wrong). I admit I don't know the proper way to deal with this (a C++ exception and PyErr_Occurred() != NULL), but it's not right like this.

Besides that, I would never print the C++ exception.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:22 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:23 Changed 4 years ago by jdemeyer

I can have a look at how to properly deal with this, but not now.

comment:24 Changed 4 years ago by rws

  • Authors Ralf Stephan deleted
  • Branch u/rws/14211 deleted
  • Commit e1374da2834bca4788084c91972e8c8c714e3b30 deleted

Now that the actual bug is fixed in future Pynac (#18360) the given doctest will not work in the future, so I removed the commit.

comment:25 Changed 4 years ago by rws

  • Branch set to u/rws/14211
  • Commit set to e1374da2834bca4788084c91972e8c8c714e3b30

Branch reset because other exceptions can be thrown.


New commits:

e1374da14211: catch Pynac exceptions

comment:26 Changed 4 years ago by rws

  • Branch u/rws/14211 deleted
  • Commit e1374da2834bca4788084c91972e8c8c714e3b30 deleted
  • Milestone changed from sage-6.7 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to positive_review

No longer necessary (doctest added in #18360)

Last edited 4 years ago by rws (previous) (diff)

comment:27 Changed 4 years ago by vbraun

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