#13447: Make libsingular multivariate polynomial rings collectable
----------------------------------------------------------------+-----------
Reporter: nbruin |
Owner:
Type: defect |
Status: needs_review
Priority: major |
Milestone: sage-5.4
Component: memleak |
Resolution:
Keywords: | Work
issues:
Report Upstream: None of the above - read trac for reasoning. |
Reviewers: Simon King
Authors: Nils Bruin, Simon King | Merged
in:
Dependencies: #11521 |
Stopgaps:
----------------------------------------------------------------+-----------
Comment (by nbruin):
Replying to [comment:57 SimonKing]:
> There are locally generated rings (in std, for example), unbeknownst to
the interpreter - they have ref zero.
>
> Singular uses `r->ref` only in `rKill` - it makes me wonder why we use
`rDelete` and not `rKill`, by the way. In fact, `rKill` deletes local
data, but `rDelete` doesn't.
If we do that, we should ensure that `rKill` has an accurate view of the
number of references. That means adjusting to Singular's conventions. If
we rKill with our current code, we'd leak because we'd call with a
positive refcount. I guess you could manually decref before calling rKill
but that's silly and misleading to future maintainers of the code.
> Generally, one should have `currRingHdl.ring == currRing`, or
`currRing==NULL` and `currRingHdl==NULL`. I just notice that he wrote
`currRingHdl.ring`, not `currRingHdl.data.uring` - is that a difference?
I don't think the former actually exists. A macro is used in the singular
code base, so Hans would not usually spell it out in full.
> Concerning debugging: If one builds Singular so that `OM_NDEBUG` is
''not'' defined, then a debug version of omalloc is used. In that way, we
would have more easily detected the original problem with `omStrDup`.
Didn't work for me but I might have just failed to properly (de)activate
those flags. Anyway, I doubt an access-after-free would be immediately
detected. That really involves unmapping or protecting memory in order to
force a segfault. I didn't see evidence that omAlloc reaches that deep
into the system. It may be that omAlloc in debugging mode does consistency
checks on every use. In that case we might have found the corruption a
little earlier. I doubt we would have had gdb point exactly at the
offending instruction. I think it would be nice for singular to update the
omAlloc headers with my changes so that (at least on linux and OSX) they
can build it with the malloc underneath. I found it very useful for this
particular issue.
> '''Conclusions'''
> Since `r->ref` does not play a rôle in libsingular, we are free to use
`r->ref` to count the number of pointers to r (not "number of pointers
minus one"). However, when calling singular interpreter functions, we must
make sure that `r->ref>0`. With our patch, we already do so - hence,
that's fine.
In principle yes, but not if we use rKill and I think it's a bad idea in
general because it might cause unforeseen problems in the future. Better
to stick with one interpretation of what the `ref` field means.
> If `currRing==NULL`, we should ''not'' create `currRingHdl`. If
`currRing!=NULL` then we should let uring point to it. the latter is
already done in my patch, '''the former should be done.'''
As you indicate, currRingHdl can be NULLed again by rKill. We need to
correct that if we need currRingHdl later.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/13447#comment:64>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
