Opened 7 years ago

Closed 7 years ago

#13450 closed defect (duplicate)

Fix refcounting of libsingular rings

Reported by: SimonKing Owned by: malb
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: commutative algebra Keywords:
Cc: nbruin, malb Merged in:
Authors: Reviewers: Nils Bruin
Report Upstream: None of the above - read trac for reasoning. Work issues:
Branch: Commit:
Dependencies: #11521 Stopgaps:

Description

While working on #715/#11521, a sporadic error has occurred:

On bsd.math (hence, OS X on Intel chips), sage -t sage/misc/cachefunc.pyx fails with signal 11. Apparently it is fine on different machines, even though with additional patches (such as #13370 or #12876) there are signal 11 in other tests and on other machines as well.

The segfault disappears when running the tests in verbose mode. It also disappears if the tests are run under gdb (but then a different error occurs, that is unrelated with the problem we are dealing with here).

Nils (see #715) was able to track the problem down to libsingular. He found (correct me if I misunderstood):

A segfault occurs in MPolynomialRing_libsingular.__init__ in the line:

        self._ring = singular_ring_new(base_ring, n, self._names, order)

Digging deeper, he came to sage/libs/singular/ring.pyx in the lines:

    _names = <char**>omAlloc0(sizeof(char*)*(len(names)))

    for i from 0 <= i < n:
        _name = names[i]
        _names[i] = omStrDup(_name)

Strange enough, the segfault occurs in omStrDup.

He found several ways to work around:

  1. In sage/libs/singular/ring.pyx, copy the strings manually, such as
       for i from 0 <= i < n:
           _name = names[i]
           j = 0
           while <bint> _name[j]:
               j+=1
           j+=1     #increment to include the 0
           copiedname =  <char*>omAlloc(sizeof(char)*(j+perturb))
           for 0 <= offset < j:
               copiedname[offset] = _name[offset]
           _names[i] = copiedname
    
    provided that the parameter perturb is at least 7.
  2. Modify manual refcounting in sage/libs/singular/ring.pyx to prevent deallocation:
         wrapped_ring = wrap_ring(_ring)
         if wrapped_ring in ring_refcount_dict:
             raise ValueError('newly created ring already in dictionary??')
    -    ring_refcount_dict[wrapped_ring] = 1
    +    ring_refcount_dict[wrapped_ring] = 2
         return _ring
    
  3. Use a strong cache for multivariate polynomial rings.

Options 2. and 3. would not be nice: The aim of #715, #11521, #12215 and #12313 was to make caches for parents weak in order to prevent memory leaks.

Since the problem disappears when deallocation of libsingular rings is prevented, it seems that the bug is in the deallocation of libsingular rings. Note that the problem is not fixed by #13145.

I don't know if it is an upstream bug or a bug in the Sage library. Hence, no idea what to report upstream.

Change History (7)

comment:1 follow-up: Changed 7 years ago by nbruin

duplicate of #13447?

comment:2 in reply to: ↑ 1 Changed 7 years ago by SimonKing

  • Status changed from new to needs_review

Replying to nbruin:

duplicate of #13447?

Sure.

comment:3 Changed 7 years ago by SimonKing

  • Reviewers set to Nils Bruin
  • Status changed from needs_review to positive_review

I guess that makes it a positive review with you as reviewer, with the suggested resolution "duplicate of #13447 ".

Last edited 7 years ago by SimonKing (previous) (diff)

comment:4 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.4 to sage-pending

comment:5 follow-up: Changed 7 years ago by SimonKing

Jeroen, why "sage-pending"? It is clear that this ticket is a duplicate of #13447.

comment:6 in reply to: ↑ 5 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-duplicate/invalid/wontfix

Replying to SimonKing:

It is clear that this ticket is a duplicate of #13447.

Then the milestone should be set to sage-duplicate/invalid/wontfix.

comment:7 Changed 7 years ago by jdemeyer

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