Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#16958 closed defect (fixed)

MPolynomial eval mem leak

Reported by: vbraun Owned by:
Priority: major Milestone: sage-6.4
Component: algebra Keywords:
Cc: malb, jpflori Merged in:
Authors: Volker Braun Reviewers: Jean-Pierre Flori
Report Upstream: N/A Work issues:
Branch: 377220e (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by vbraun)

When a libsingular polynomial evaluates to a constant we leak the resulting libsingular poly.

Change History (19)

comment:1 Changed 8 years ago by vbraun

  • Branch set to u/vbraun/mpolynomial_eval_mem_leak

comment:2 Changed 8 years ago by vbraun

  • Authors set to Volker Braun
  • 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

New commits:

14061a1Fix mem leak when polynomial evaluates to constant

comment:3 Changed 8 years ago by jpflori

Can you add a doctest showing the leak is fixed?

comment:4 follow-up: Changed 8 years ago by vbraun

  • Status changed from needs_review to needs_work

Mmm got another segfault elsewhere..

comment:5 Changed 8 years ago by SimonKing

  • 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 SimonKing

  • 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 nbruin

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 follow-up: Changed 8 years ago by vbraun

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 SimonKing

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 git

  • Commit changed from 14061a1d40e8a27d86eac6dd91559b0131f9fe55 to 6b5a4c4f5b675fd2e42561c9ed7c145215ae17d8

Branch pushed to git repo; I updated commit sha1. New commits:

6b5a4c4Normalize polynomials after fast_map to avoid segfault

comment:11 Changed 8 years ago by git

  • Commit changed from 6b5a4c4f5b675fd2e42561c9ed7c145215ae17d8 to 377220e947cf36ab019af2d243db89441a64ee7e

Branch pushed to git repo; I updated commit sha1. New commits:

377220eDoctest that the memory leak is gone

comment:12 follow-up: Changed 8 years ago by vbraun

  • Cc malb added
  • Status changed from needs_work to needs_review

Singular's fast_map returns non-normalized 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 malb

Replying to vbraun:

Singular's fast_map returns non-normalized 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.

I don't know the answer, maybe ask on [libsingular-devel]?

comment:15 Changed 8 years ago by jpflori

  • Cc jpflori added

comment:16 Changed 7 years ago by jpflori

  • Reviewers set to Jean-Pierre 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 vbraun

  • 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 SimonKing

  • 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 SimonKing

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!

Note: See TracTickets for help on using tickets.