Don't store environ argument to GAP_Initialize if it is the same as the system environ

[embray]

This is a problem particularly for using GAP as a library, since one will typically call GAP_Initialize like:

extern char** environ;

void init() {
    char** argv;
    int argc;
    /* ... */
    GAP_Initialize(&argc, argv, environ);
}

This results in bugs like those explained on this page, since GAP_Initialize stores a copy of this argument to its own sysenviron global.

Alternatively, we do away with the sysenviron variable completely and just use environ. I suspect the current implementation is very old, and I don't know if GAP cares about supporting non-POSIX-compliant systems anymore.

[ChrisJefferson]

My only (small) comment is I wouldn't bother checking if we are on a POSIX system -- we generally assume that we are on a "sensible" POSIX OS nowadays. If we find we need to test, we'd probably be better off using autoconf (and I don't think that's required here).

Once this is merged, and backported to 4.10, I'm tempted to just remove all this and always use the environ global variable (possibly leaving an optional 3rd argument to GAP_Initalize for backwards compatibility), but that's another PR.

[fingolfin]

I agree with @ChrisJefferson and would go further: would you mind changing this PR to instead remove sysenviron? Just replace its first four uses by environ, and get rid of the fifth (which is sysenviron = environ). That leaves the place which currently iterates over sysenviron and that assumes it is non-null. But that's of course easily fixed, e.g. by changing for (i = 0; sysenviron[i]; i++) { to for (i = 0; sysenviron && sysenviron[i]; i++) {.

[fingolfin]

Well, and then get rid of the environ arg to InitializeGap.

Hmm, well, all that at least unless somebody can think of a good reason why a client might want to pass a custom environ to GAP instead? And I mean "good": of course I can invent silly stuff, but that's not the point. I mean: "a reason why SageMath / GAPJulia / any other actually existing or at least planned interface would need it". Compatibility with a hypothetical future OS or interface is something I'd prefer to deal with when it turns up.

[embray]

Well, and then get rid of the environ arg to InitializeGap.

Yes, that would be necessary as well. Which IMO is fine, it's not officially part of the "API" anyways. But then it would also mean changing the API again to remove it from GAP_Initialize. As that's still sort of "beta" maybe that's ok anyways. That, or keep it but ignore it. Up to you guys.

I don't believe there's really any good reason to be able to pass a custom environment array here. Maybe for testing purposes but even that's a stretch.

By the way, the roughly equivalent code in CPython is here: https://github.com/python/cpython/blob/master/Modules/posixmodule.c#L1326 And there's an interesting note about not being able to use environ directly. I'm not entirely sure if that's still true though: @dimpase works mostly on MacOS I think and didn't report any problems like this...?

[dimpase]

On Thu, 13 Dec 2018 15:15 E. M. Bray ***@***.*** wrote: I'm not entirely sure if that's still true though: @dimpase <https://github.com/dimpase> works mostly on MacOS I think and didn't report any problems like this...?

While I own an 8 y.o. OSX laptop and can check things on it if really needed, I certainly not do it often.

[fingolfin]

Ahh, good point about _NSGetEnviron, I forgot about that. Indeed, this is still relevant today. To quote man environ on OS X:

Shared libraries and bundles don't have direct access to environ, which is only available to the loader ld(1) when a complete program is being linked. The environment routines can still be used, but if direct access to environ is needed, the _NSGetEnviron() routine, defined in <crt_externs.h>, can be used to retrieve the address of environ at runtime.

But that's easy enough to deal with, of course, e.g. by "borrowing" that code from cpython.

[fingolfin]

And yes, I would also remove the environ argument from GAP_Initialize (but let's hear what @markuspf and @ChrisJefferson think?)

[dimpase]

We stepped on this rake (you know, a totally crazy segfault out of nowhere) while working on Sage interface to libgap --- fortunately @embray was quick to discover the root of the problem.

[ChrisJefferson]

My only comment is if we want something small (like this PR) for backporting to 4.10 quickly and safely (if this pr is enough for sage, sounds like it might not be on mac?) Then tear the whole lot out for 4.11.

[embray]

Perhaps for 4.10.1 this is good enough as it maintains backwards compat as well, but I can open a separate, more destructive PR. I'll also add the _NSGetEnviron stuff in either case; ugh.

@dimpase Sorry, for some reason I mistakenly thought you were testing on OSX sometimes; maybe I was thinking of John Palmieri.

[embray]

I found this bit in GCC, which is a bit nicer IMO, and good enough for most purposes: https://github.com/gcc-mirror/gcc/blob/master/include/environ.h

[fingolfin]

I personally would not worry too much about the state of things in GAP 4.10: there, libgap is not advertised as a stable, fixed API or ABI anyway. The only reason to talk about it at all is that 4.10.1 and 4.10.2 most likely will be released long before 4.11, so a version suitable for use with SageMath would be available earlier. OTOH, the target for GAP 4.11 is May 2019, so perhaps that's soon enough for you anyway, in which case I'd just forget about 4.10.x.

[embray]

the target for GAP 4.11 is May 2019, so perhaps that's soon enough for you anyway

It's not soon enough. I'm really hoping to have something viable for inclusion in the freeze for the next Debian release. We're running out of time on that, but it's quite close (and downstream may be able to accept some additional patches if necessary).

[fingolfin]

Well, when is that freeze? It'll only matter if GAP 4.10.1 is released in time for it, after all...

[dimpase]

https://lists.debian.org/debian-devel-announce/2018/09/msg00004.html

(I guess that having 4.10.1 by 2019-01-12 might be ideal, but perhaps 2019-02-12 is still possible to get it in. Not sure).

[fingolfin]

Huh, not sure we can manage Jan 12 (pining @alex-konovalov )

[olexandr-konovalov]

@fingolfin Please use milestones to mark what we need for GAP 4.10.1, then I can tell.

[fingolfin]

@alex-konovalov my question is the other way around: assuming stable-4.10 was "ready" (as in: nothing to be backported to it anymore), say, next monday: then when could we get 4.10.1 released? I mean, this requires a lot of testing and stuff, doesn't it? And christmas and new year are coming up...

[embray]

Next monday is in like 3 days; I'm not going to be able to do what I need to do for that, esp since I won't likely work on it over the weekend. But certainly the sooner the better. Like I said, it will probably also be possible to include some patches in the Debian package if it's needed at the last minute, but the smaller those patches the better.

[fingolfin]

@embray oh, I didn't mean to suggest this'd be ready on monday; rather, I was wondering whether (or rather, doubting that) we had a chance for a release before 2019-01-12 even under this "best case scenario...

[olexandr-konovalov]

Well, right now weekly Jenkins tests of GAP 4.10.11 release candidates go well, and Travis tests with badges at https://github.com/gap-system/gap-distribution also go well. The milestone for GAP 4.11 has all things closed. So if it would be now, one should add a section to the Changes manual with an overview of items under this milestone, and publish the release (perhaps declare a freeze now, but publish it during the first working week of in January).

However, that may be not what @embray is interested - there are also:

• merged but not backported: Add support for writing to ERROR_OUTPUT from kernel code #3043
• not yet merged: Allow disabling the GAP kernel's signal handling when using the API #3072
• and this PR itself

and it seems to me that without them GAP 4.10.1 would not be very helpful to @embray (please correct me if even having some of these would be useful). In the very best case scenario (i.e. all Jenkins/Travis tests pass after merging/backporting, and checks on SageMath's side pass too), a chance of least publishing the archives on web by 2019-01-12 remains.

[fingolfin]

@alex-konovalov yes, of course also other changes are needed. Once again: my question was based on the hypothetical assumption that we'd be ready on Monday. Of course we won't be, that's clear.