Pari initialization segfaults in Cygwin since #22633

[embray]

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.

[jdemeyer]

Replying to embray:

It seems that MAP_FIXED is not working correctly for some reason

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 and res after that mmap() call.

[embray]

Yes, that's what I'm trying to do now.

[jdemeyer]

I guess that MAP_FIXED is not commonly used, so it wouldn't be so surprising that it is buggy in Cygwin.

[embray]

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.

[embray]

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?

[jdemeyer]

Replying to embray:

The only problem is the munmap followed by mmap 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.

[embray]

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.

[embray]

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.

[jdemeyer]

Thanks for the analysis. Do you have a patch ready?

[embray]

Incoming.

[jdemeyer]

Remember to base this ticket on top of #22675.

[embray]

Yep, that's the plan.

[embray]

Still waiting on some testing.

---

New commits:

​543c4a1	Upgrade PARI to version 2.9.2
​ab20b1b	Another patch to Pari's stack allocation to fix a bug on Cygwin: https://trac.sagemath.org/ticket/22810

[git]

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

[embray]

Seems to work now.

[tscrim]

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.

[jdemeyer]

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.

[embray]

I did all of the above, but Travis should confirm if possible.

[embray]

Please by my CI, Travis (sorry, you probably get that all the time...)

[tscrim]

I will be your CI. :P

Built okay and tests pass.

[embray]

This still needs to be reported upstream, however.

[jdemeyer]

Right. Should I?