Opened 13 years ago

Closed 13 years ago

#5839 closed defect (fixed)

[with patch, positive review] MPolynomialRing_libsingular __dealloc__ is buggy, can lead to crash

Reported by: cwitty Owned by: cwitty
Priority: critical Milestone: sage-4.0
Component: commutative algebra Keywords:
Cc: malb Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

In __dealloc__, if currRing is NULL on entry, then currRing will be the ring we just deleted on exit. The patch fixes this bug, so that currRing never points to freed memory.

It took me quite a while to come up with a small reproducible test case for the problem; here it is. (This test case is also in the patch, as a doctest.) 

import gc
from sage.rings.polynomial.multi_polynomial_libsingular import MPolynomialRing_libsingular
R1 = MPolynomialRing_libsingular(GF(5), 2, ('x', 'y'), TermOrder('degrevlex', 2))
R2 = MPolynomialRing_libsingular(GF(11), 2, ('x', 'y'), TermOrder('degrevlex', 2))
R3 = MPolynomialRing_libsingular(GF(13), 2, ('x', 'y'), TermOrder('degrevlex', 2))
gc.collect()
foo = R1.gen(0)
del foo
del R1
gc.collect()
del R2
gc.collect()
del R3
gc.collect()

Attachments (1)

fix-mp-libsingular-dealloc.patch​ (2.1 KB) - added by cwitty 13 years ago.

Download all attachments as: .zip

Change History (10)

Changed 13 years ago by cwitty

comment:1 Changed 13 years ago by malb

  • Summary changed from [with patch, needs review] MPolynomialRing_libsingular __dealloc__ is buggy, can lead to crash to [with patch,positive review] MPolynomialRing_libsingular __dealloc__ is buggy, can lead to crash

Doctests pass, patch reads good.

comment:2 Changed 13 years ago by mabshoff

  • Milestone changed from sage-3.4.1 to sage-3.4.2

comment:3 Changed 13 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed
  • Summary changed from [with patch,positive review] MPolynomialRing_libsingular __dealloc__ is buggy, can lead to crash to [with patch, positive review] MPolynomialRing_libsingular __dealloc__ is buggy, can lead to crash

Merged in Sage 3.4.2.alpha0.

Cheers,

Michael

comment:4 Changed 13 years ago by mabshoff

  • Resolution fixed deleted
  • Status changed from closed to reopened

Mmmh, I seem to be seeing random doctest failure with 3.4.1 + only this patch merged, so I am reopening this ticket for now. Can someone do extensive testing on sage.math to see if they can reproduce it?

Cheers,

Michael

comment:5 Changed 13 years ago by mabshoff

Ok, given that this causes issues for me I am changing it to 'needs work' for now.

Cheers,

Michael

comment:6 Changed 13 years ago by mabshoff

  • Summary changed from [with patch, positive review] MPolynomialRing_libsingular __dealloc__ is buggy, can lead to crash to [with patch, needs work] MPolynomialRing_libsingular __dealloc__ is buggy, can lead to crash

comment:7 Changed 13 years ago by cwitty

What sort of errors? (Crashes? Wrong answers? Any particular files?)

comment:8 Changed 13 years ago by mabshoff

  • Summary changed from [with patch, needs work] MPolynomialRing_libsingular __dealloc__ is buggy, can lead to crash to [with patch, positive review] MPolynomialRing_libsingular __dealloc__ is buggy, can lead to crash

I have come to believe that the issues I saw were due to other issues, so I am reinstating the positive review. But I will do some more extensive testing before merging this. Sorry for the noise.

Cheers,

Michael

comment:9 Changed 13 years ago by mabshoff

  • Milestone changed from sage-4.0.1 to sage-4.0
  • Resolution set to fixed
  • Status changed from reopened to closed

Merged in Sage 4.0.alpha0.

Cheers,

Michael

Note: See TracTickets for help on using tickets.