#16958 closed defect (fixed)
MPolynomial eval mem leak
Reported by:  vbraun  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  algebra  Keywords:  
Cc:  malb, jpflori  Merged in:  
Authors:  Volker Braun  Reviewers:  JeanPierre Flori 
Report Upstream:  N/A  Work issues:  
Branch:  377220e (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
When a libsingular polynomial evaluates to a constant we leak the resulting libsingular poly
.
Change History (19)
comment:1 Changed 8 years ago by
 Branch set to u/vbraun/mpolynomial_eval_mem_leak
comment:2 Changed 8 years ago by
 Commit set to 14061a1d40e8a27d86eac6dd91559b0131f9fe55
 Component changed from PLEASE CHANGE to algebra
 Description modified (diff)
 Status changed from new to needs_review
 Type changed from PLEASE CHANGE to defect
comment:3 Changed 8 years ago by
Can you add a doctest showing the leak is fixed?
comment:4 followup: ↓ 7 Changed 8 years ago by
 Status changed from needs_review to needs_work
Mmm got another segfault elsewhere..
comment:5 Changed 8 years ago by
 Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.
Shouldn't those fixes be reported upstream?
comment:6 Changed 8 years ago by
 Report Upstream changed from Not yet reported upstream; Will do shortly. to N/A
Ah, sorry, spoke to soon: The leak was in Sage library, not in Singular.
comment:7 in reply to: ↑ 4 Changed 8 years ago by
Replying to vbraun:
Mmm got another segfault elsewhere..
That doesn't surprise me. We tried to properly interface with libsingular's reference counting before and, while we came close, it ended up being a rather nightmarish experience. See #13447.
I am still quite hopeful that this can be fixed *if the right experts get in the same room* for a couple of days (either that or people will find out if libsingular's reference counting is fundamentally broken).
(note that #13447 is about garbage collecting rings, which are probably a little more complicated data structures. Nonetheless, there may well be components in common).
comment:8 followup: ↓ 9 Changed 8 years ago by
I'm pretty sure I'm doing the right thing, you get a pointer to a poly so duh of course it needs to be deallocated. There are only two doctest failures in Sage, so I'm mildly optimistic. I'm probably uncovering bugs elsewhere...
Simon, did you make any progress towards a debug build?
comment:9 in reply to: ↑ 8 Changed 8 years ago by
Replying to vbraun:
Simon, did you make any progress towards a debug build?
I got answer from Hans Schönemann. But I don't know yet if it works. To be discussed on the other ticket.
comment:10 Changed 8 years ago by
 Commit changed from 14061a1d40e8a27d86eac6dd91559b0131f9fe55 to 6b5a4c4f5b675fd2e42561c9ed7c145215ae17d8
Branch pushed to git repo; I updated commit sha1. New commits:
6b5a4c4  Normalize polynomials after fast_map to avoid segfault

comment:11 Changed 8 years ago by
 Commit changed from 6b5a4c4f5b675fd2e42561c9ed7c145215ae17d8 to 377220e947cf36ab019af2d243db89441a64ee7e
Branch pushed to git repo; I updated commit sha1. New commits:
377220e  Doctest that the memory leak is gone

comment:12 followup: ↓ 13 Changed 8 years ago by
 Cc malb added
 Status changed from needs_work to needs_review
Singular's fast_map
returns nonnormalized values (e.g. rational coefficients are not in standard form). As far as I know, this shouldn't be a problem yet we still segfault in the p_Delete
. Adding a p_Normalize
avoids the segfault. Perhaps there is a subtle bug / hidden assumption in fast_map
that requires the normalization, I don't know.
comment:13 in reply to: ↑ 12 Changed 8 years ago by
Replying to vbraun:
Singular's
fast_map
returns nonnormalized values (e.g. rational coefficients are not in standard form). As far as I know, this shouldn't be a problem yet we still segfault in thep_Delete
. Adding ap_Normalize
avoids the segfault. Perhaps there is a subtle bug / hidden assumption infast_map
that requires the normalization, I don't know.
I don't know the answer, maybe ask on [libsingulardevel]?
comment:14 Changed 8 years ago by
I asked at https://groups.google.com/d/msg/libsingulardevel/NV_j8Fugi4o/a7OdyHQSFf4J but didn't get a reply
comment:15 Changed 8 years ago by
 Cc jpflori added
comment:16 Changed 7 years ago by
 Reviewers set to JeanPierre Flori
 Status changed from needs_review to positive_review
Works fine for me. For sure the need for normalize does not really make sense, but it makes the situation a little better here.
comment:17 Changed 7 years ago by
 Branch changed from u/vbraun/mpolynomial_eval_mem_leak to 377220e947cf36ab019af2d243db89441a64ee7e
 Resolution set to fixed
 Status changed from positive_review to closed
comment:18 Changed 7 years ago by
 Commit 377220e947cf36ab019af2d243db89441a64ee7e deleted
I wonder if that fixes the memory leak for the letterplace algebra, that I noticed while working on #17435. If it doesn't, I should open a new ticket at some point...
comment:19 Changed 7 years ago by
Why has the commit information been written into the "branch" field when Volker closed the ticket? And why has my post resulted in a deletion of the commit field? I didn't touch it!
New commits:
Fix mem leak when polynomial evaluates to constant