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:  sageduplicate/invalid/wontfix 
Component:  symbolics  Keywords:  
Cc:  Merged in:  
Authors:  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
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
 Milestone changed from sage5.11 to sage5.12
comment:2 Changed 5 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:3 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:4 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:5 Changed 4 years ago by
 Priority changed from major to critical
comment:6 Changed 4 years ago by
 Description modified (diff)
 Milestone changed from sage6.4 to sage6.5
comment:7 Changed 4 years ago by
 Milestone changed from sage6.5 to sage6.7
 Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.
comment:8 followup: ↓ 11 Changed 4 years ago by
 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?
comment:9 Changed 4 years ago by
 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
 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 ; followups: ↓ 12 ↓ 17 Changed 4 years ago by
Replying to rws:
which is the same you get with
hash(matrix())
, so quite sensible. What more? Why are matrices generated byvector*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
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 lhsrhs
. 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
comment:13 Changed 4 years ago by
comment:14 Changed 4 years ago by
 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
 Report Upstream changed from Reported upstream. Developers deny it's a bug. to N/A
comment:16 Changed 4 years ago by
 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
 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)) 0but
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:
b9698e0  14211: catch exceptions from Expression.__nonzero__

comment:18 Changed 4 years ago by
 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
 Branch changed from u/rws/crash_due_to_missing_cython_exception_handling_for_relational_to_bool__ to u/rws/14211
comment:20 followup: ↓ 21 Changed 4 years ago by
 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:
e1374da  14211: catch Pynac exceptions

comment:21 in reply to: ↑ 20 Changed 4 years ago by
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.
comment:22 Changed 4 years ago by
 Status changed from needs_review to needs_work
comment:23 Changed 4 years ago by
I can have a look at how to properly deal with this, but not now.
comment:24 Changed 4 years ago by
 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
 Branch set to u/rws/14211
 Commit set to e1374da2834bca4788084c91972e8c8c714e3b30
Branch reset because other exceptions can be thrown.
New commits:
e1374da  14211: catch Pynac exceptions

comment:26 Changed 4 years ago by
 Branch u/rws/14211 deleted
 Commit e1374da2834bca4788084c91972e8c8c714e3b30 deleted
 Milestone changed from sage6.7 to sageduplicate/invalid/wontfix
 Status changed from needs_work to positive_review
No longer necessary (doctest added in #18360)
comment:27 Changed 4 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
Reported as https://github.com/pynac/pynac/issues/34