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.

Depends on #22675

Upstream: Not yet reported upstream; Will do shortly.

CC: @jdemeyer

Component: porting: Cygwin

Keywords: windows cygwin pari mmap

Author: Erik Bray

Branch: 00a5d73

Reviewer: Travis Scrimshaw, Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/22810

[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]

comment:2

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

[jdemeyer]

comment:3

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

[embray]

comment:4

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]

comment:5

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]

comment:6

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.

[jdemeyer]

Dependencies: #22675

[embray]

comment:7

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]

comment:9

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]

comment:10

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

[embray]

comment:11

Incoming.

[jdemeyer]

comment:12

Remember to base this ticket on top of #22675.

[embray]

comment:13

Yep, that's the plan.

[embray]

comment:14

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://github.com/sagemath/sage/issues/22810

[embray]

Commit: ab20b1b

[embray]

Author: Erik Bray

[embray]

Branch: u/embray/cygwin/ticket-22810

[sagetrac-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://github.com/sagemath/sage/issues/22810

[sagetrac-git]

Changed commit from ab20b1b to 00a5d73

[embray]

comment:16

Seems to work now.

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:17

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]

comment:18

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]

comment:19

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

[embray]

comment:20

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

[tscrim]

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Jeroen Demeyer

[tscrim]

comment:21

I will be your CI. :P

Built okay and tests pass.

[vbraun]

Changed branch from u/embray/cygwin/ticket-22810 to 00a5d73

[embray]

Changed commit from 00a5d73 to none

[embray]

Upstream: Not yet reported upstream; Will do shortly.

[embray]

comment:23

This still needs to be reported upstream, however.

[jdemeyer]

comment:24

Right. Should I?