#22810 closed defect (fixed)
Pari initialization segfaults in Cygwin since #22633
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-8.0 |
Component: | porting: Cygwin | Keywords: | windows cygwin pari mmap |
Cc: | jdemeyer | Merged in: | |
Authors: | Erik Bray | Reviewers: | Travis Scrimshaw, Jeroen Demeyer |
Report Upstream: | Not yet reported upstream; Will do shortly. | Work issues: | |
Branch: | 00a5d73 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | #22675 | Stopgaps: |
Description
Sorry, I thought I had tested the last version of the patch in #22633. But I think I just tested it in PARI/GP, but not in Sage itself, since my Sage build was broken at the time for other reasons.
The offending line is here. It seems that MAP_FIXED
is not working correctly for some reason, but the resulting call to pari_err
segfaults because the pari mainstack is left in an invalid state.
I'm not sure if this is a bug in Cygwin or what, but I'm trying to put together a simplified test case.
Change History (24)
comment:1 in reply to: ↑ description Changed 5 years ago by
comment:2 Changed 5 years ago by
Yes, that's what I'm trying to do now.
comment:3 Changed 5 years ago by
I guess that MAP_FIXED
is not commonly used, so it wouldn't be so surprising that it is buggy in Cygwin.
comment:4 Changed 5 years ago by
The problem is not so much MAP_FIXED
itself--it works in a different context.
In this case the mmap
in pari_mainstack_mreset
is failing unless I call munmap(addr, s)
first. If I understand correctly that should be fine, because we're not using that memory anyways right now if we're setting it to PROT_NONE
.
comment:5 follow-up: ↓ 6 Changed 5 years ago by
I also found some relevant discussion here: http://cygwin.1069669.n5.nabble.com/mmap-MAP-FIXED-vs-mprotect-td98125.html But I think it's fine as long as you munmap()
first--that will set those pages to unused/unresreved, and then they can be re-reserved. The only problem is the munmap
followed by mmap
will not be atomic. But how likely is that to be a problem in practice?
comment:6 in reply to: ↑ 5 Changed 5 years ago by
- Dependencies set to #22675
Replying to embray:
The only problem is the
munmap
followed bymmap
will not be atomic. But how likely is that to be a problem in practice?
With multi-threading, there is a non-zero chance of things going wrong.
For the record, calling mmap()
with MAP_FIXED
on a already-mapped region is specified by POSIX to replace an existing mapping. So if Cygwin does not support that, I would consider that a bug.
comment:7 Changed 5 years ago by
Right, I think that would be a bug too--the thread I linked to above has some comment about that.
Honestly, I might just add a patch that reverts to the old mprotect-based approach on Cygwin.
comment:8 Changed 5 years ago by
- Priority changed from major to blocker
comment:9 Changed 5 years ago by
I spent some more time poking at this, and what I found is that it suffices on Cygwin to mprotect(..., PROT_NONE)
rather than re-mmap with PROT_NONE and MAP_FIXED, which is in fact buggy (although a known issue).
This means, unfortunately, having to put an #ifdef __CYGWIN__
in pari_mainstack_mreset
, to have it use mprotect
instead of mmap
, but other than that I think that is the only change needed. In Cygwin, mprotect(..., PROT_NONE)
will actually decommit the memory, which is what we want.
comment:10 Changed 5 years ago by
Thanks for the analysis. Do you have a patch ready?
comment:11 Changed 5 years ago by
Incoming.
comment:12 Changed 5 years ago by
Remember to base this ticket on top of #22675.
comment:13 Changed 5 years ago by
Yep, that's the plan.
comment:14 Changed 5 years ago by
- Branch set to u/embray/cygwin/ticket-22810
- Commit set to ab20b1b912075256314c7ec8c94bf799a8a76428
comment:15 Changed 5 years ago by
- Commit changed from ab20b1b912075256314c7ec8c94bf799a8a76428 to 00a5d73376817860b85c1abcb1a10203e28d897a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
00a5d73 | Another patch to Pari's stack allocation to fix a bug on Cygwin: https://trac.sagemath.org/ticket/22810
|
comment:17 follow-up: ↓ 18 Changed 5 years ago by
- Reviewers set to Travis Scrimshaw
Jeroen, any more comments on this? Since this worked for me on Cygwin, I would set this to a positive review. However, you know much more about Pari than I do.
comment:18 in reply to: ↑ 17 Changed 5 years ago by
Replying to tscrim:
Since this worked for me on Cygwin
Define "worked". Did you run the PARI testsuite? And the Sage doctests in src/sage/libs/*pari*
?
If both of these pass, then it's fine for me.
comment:19 Changed 5 years ago by
I did all of the above, but Travis should confirm if possible.
comment:20 Changed 5 years ago by
Please by my CI, Travis (sorry, you probably get that all the time...)
comment:21 Changed 5 years ago by
- Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Jeroen Demeyer
- Status changed from needs_review to positive_review
I will be your CI. :P
Built okay and tests pass.
comment:22 Changed 5 years ago by
- Branch changed from u/embray/cygwin/ticket-22810 to 00a5d73376817860b85c1abcb1a10203e28d897a
- Resolution set to fixed
- Status changed from positive_review to closed
comment:23 Changed 5 years ago by
- Commit 00a5d73376817860b85c1abcb1a10203e28d897a deleted
- Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.
This still needs to be reported upstream, however.
comment:24 Changed 5 years ago by
Right. Should I?
Replying to embray:
The question now becomes: why?
Does everything work fine if you remove the line
if (res != addr) pari_err(e_MEM);
For debugging, it would be useful to print the values of
addr
,s
andres
after thatmmap()
call.