Opened 10 years ago

Closed 10 years ago

#13746 closed defect (fixed)

Do not delete a borrowed reference to reduction strategies in pbori

Reported by: Jean-Pierre Flori Owned by: tbd
Priority: blocker Milestone: sage-5.5
Component: memleak Keywords: segfault
Cc: Simon King, Alexander Dreyer Merged in: sage-5.5.rc1
Authors: Jean-Pierre Flori, Simon King Reviewers: Alexander Dreyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

In #12313, it was discovered in a painful way that Sage tries to deallocate twice memory allocated for reduction strategies in pbori.

As this fix is now needed by other tickets like #715, I've put the fix on an independent ticket.

Attachments (1)

trac_12313_quit_sage.patch (2.5 KB) - added by Jean-Pierre Flori 10 years ago.
Patch from #12313 with only changes concerning trac ticket numbers

Download all attachments as: .zip

Change History (12)

Changed 10 years ago by Jean-Pierre Flori

Attachment: trac_12313_quit_sage.patch added

Patch from #12313 with only changes concerning trac ticket numbers

comment:1 Changed 10 years ago by Jean-Pierre Flori

Component: PLEASE CHANGEmemleak
Status: newneeds_review

comment:2 Changed 10 years ago by Simon King

OK, you took the patch from #12313 and removed the bits that now belong to #13741, right?

I don't recall who wrote most of the code in the patch - in other words: Who can be reviewer for it?

comment:3 in reply to:  2 Changed 10 years ago by Jean-Pierre Flori

Replying to SimonKing:

OK, you took the patch from #12313 and removed the bits that now belong to #13741, right?

No, the two patches were orthogonal, they touched different part of quit_sage. I'm not completely sure the additional garbage collection we introduce here is really needed, but it cannot hurt for sure (or it can reveal nasty bugs :) which is good).

I don't recall who wrote most of the code in the patch - in other words: Who can be reviewer for it?

I think the patch was already positively reviewed during the process of reviewing #12313, so you or Nils could set this ticket to positive review, I just don't feel doing it as I've opened the ticket.

comment:4 Changed 10 years ago by Jean-Pierre Flori

And so this is exactly the same patch as in #12313, except that:

  • I've replace 12313 by 13746 in the commit message
  • I've replace 12313 by 12313 and 13746 in some comment in the code redirecting the reader to Sage's trac

comment:5 Changed 10 years ago by Simon King

Replying to jpflori:

Replying to SimonKing:

OK, you took the patch from #12313 and removed the bits that now belong to #13741, right?

No, the two patches were orthogonal,

Sorry, you are totally right! What I moved to #13471 came from #12215 or so...

I'm not completely sure the additional garbage collection we introduce here is really needed, but it cannot hurt for sure (or it can reveal nasty bugs :) which is good).

Indeed!

I think the patch was already positively reviewed during the process of reviewing #12313, so you or Nils could set this ticket to positive review, I just don't feel doing it as I've opened the ticket.

Not before running a full test...

comment:6 Changed 10 years ago by Alexander Dreyer

Cc: Alexander Dreyer added

comment:7 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.6sage-5.5
Priority: criticalblocker

comment:8 Changed 10 years ago by Alexander Dreyer

I could probably review it, since I've already discussed it previously. The patch looks sane, yet I don't understand, why it fails on the patchbot. (Is this related to something else?)

comment:9 in reply to:  8 Changed 10 years ago by Simon King

Replying to AlexanderDreyer:

I could probably review it, since I've already discussed it previously. The patch looks sane, yet I don't understand, why it fails on the patchbot. (Is this related to something else?)

My guess is that the error is unrelated (it seems to come from a killed Gap session). To be on the safe side, I've just kicked the patchbot, so that it should now repeat the tests.

comment:10 Changed 10 years ago by Alexander Dreyer

Reviewers: Alexander Dreyer
Status: needs_reviewpositive_review

Looks good!

comment:11 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.5.rc1
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.