Opened 7 years ago

Closed 7 years ago

#13746 closed defect (fixed)

Do not delete a borrowed reference to reduction strategies in pbori

Reported by: jpflori Owned by: tbd
Priority: blocker Milestone: sage-5.5
Component: memleak Keywords: segfault
Cc: SimonKing, AlexanderDreyer 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:

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 jpflori 7 years ago.
Patch from #12313 with only changes concerning trac ticket numbers

Download all attachments as: .zip

Change History (12)

Changed 7 years ago by jpflori

Patch from #12313 with only changes concerning trac ticket numbers

comment:1 Changed 7 years ago by jpflori

  • Component changed from PLEASE CHANGE to memleak
  • Status changed from new to needs_review

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

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 7 years ago by 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, 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 7 years ago by jpflori

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 7 years ago by SimonKing

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 7 years ago by AlexanderDreyer

  • Cc AlexanderDreyer added

comment:7 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.6 to sage-5.5
  • Priority changed from critical to blocker

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

comment:9 in reply to: ↑ 8 Changed 7 years ago by SimonKing

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 7 years ago by AlexanderDreyer

  • Reviewers set to Alexander Dreyer
  • Status changed from needs_review to positive_review

Looks good!

comment:11 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.5.rc1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.