Proposal: remove unreliable "abused getenv" test and related code

[memreflect]

Unlike some older versions, es no longer replaces the system getenv implementation unless the configure test determines getenv is "abused" by the system. As I understand it, this test previously made sense because esdump would dump core as a result of es having its own getenv implementation.

However, a Debian patch changed the code such that getenv will never be replaced by es unless that abuse is detected at configure time, yet there is no point in detecting that abuse unless you want to replace getenv in the first place. The Debian patch unintentionally introduced a chicken-and-egg problem.

Moreover, the configure test is currently unreliable:

• Debian 11 (Bullseye): Not abused—except GNU's getopt function does call the system implementation of getenv. Simply put, the test reports a false negative, so the replacement getenv will never be used.
• OpenBSD 7.0: Not abused—except the C runtime does call the system implementation of getenv before main begins execution. Again, the test reports a false negative, so the replacement getenv will never be used.
• FreeBSD 13.0 and NetBSD 9.2: Abused—the system implementation of getenv is called before main and the configure test correctly detects this. Due to the code changes however, either the system implementation is always used if es_cv_abused_getenv=no is manually set at configure time or the replacement is always used if es_cv_abused_getenv=no is not manually set. Either way, the test is pretty much unnecessary as only one getenv implementation is ever used. One might consider this case to be a false positive since the system getenv does not actually cause any trouble.

Thoughts?

[wryun]

Ah, sorry for merging that Debian stuff without actually checking what it was doing, and thanks for doing the analysis here.

Do I understand correctly though that this is currently broken on NetBSD/FreeBSD? Or has the core dumping problem gone away for other reasons?

[memreflect]

(I apologize for my earlier misleading remark. The Debian patch actually was not the cause, so merging it was fine. The Debian package maintainer noticed that esgetenv would never be called unless getenv was abused, so they moved the preprocessor conditional, resulting in omission of a useless function even if compiler optimizations were disabled.)

What I'm suggesting is that the abused getenv test is no longer reliable, failing on systems where getenv is perfectly adequate (FreeBSD/NetBSD) and not failing on systems that were intended to be caught by the test in the first place (Debian/OpenBSD).

A bit of history...

The function esgetenv was originally named getenv. It uses es data structures and garbage collection, so es must be initialized enough for it to be used. Readline requires an accurate value for the TERM environment variable, so a replacement getenv function was devised that would report the current value of TERM, and any time you change TERM, fn-set-TERM in initial.es would call $&resetterminal to ensure rl_reset_terminal(NULL) was called, which would invoke the replacement getenv. This was fine as readline wouldn't actually be used until the shell was ready to print a prompt. The fact that it was named getenv, however, caused core dumps when the new getenv was called by getopt or before main on certain systems, i.e. before es was initialized enough for the function to be usable.

Now here's the other part of the problem: on Debian, OpenBSD, and other systems where es_cv_abused_getenv=no, the new replacement getenv function contributed in 0.9-beta1 will not even be compiled in, and the value of TERM prior to shell startup continues to be used by readline, even when the value of TERM changes; this can be verified by adding a eprint("%s\n", getenv("TERM")); line after rl_reset_terminal(NULL); in input.c to see what readline used. This is because es never modifies its environment using setenv or putenv. External utilities like clear, tput, and env only use the value of TERM you set because they are executed using execve with the array of environment variables that es passes to the utility.

I suppose you might say that it's working as intended on FreeBSD/NetBSD as getenv is correctly detected as "abused", resulting in the replacement of getenv with the one in es. This way, the correct TERM value is reported to readline, but that means things are not working as intended on Debian/OpenBSD. In starting this discussion, I thought everything would continue working as it's supposed to, even if the abused getenv test and related code was removed, but investigating the TERM environment variable revealed the incorrect TERM being used by readline.

Passing the correct value of TERM to rl_reset_terminal would make things work on all systems. This way, there is no need to replace getenv and no reason to determine whether it is abused or not. I'd be happy to submit a PR that passes the value of TERM to rl_reset_terminal to fix this issue, though some might rightly argue the real fix would be to rework code relating to environment variables.

[wryun]

Thanks for that detailed explanation. I think I have a better grasp of what's going on now.

I guess the original concern was that you don't know what env vars readline or any linked in libraries might be interested in. But, hey, we're controlling the linking, and I can't imagine what else we'd need to link, so addressing this in an ad-hoc way seems reasonable.

However, just passing TERM to rl_reset_terminal isn't quite sufficient - readline also looks at other variables (HOME/LC_ALL/COLUMNS/LINES/INPUTRC). Most look like they're only startup things, but HOME definitely has a runtime effect (~/ completion).

Rather than ifdefing this thing out, can we not have some kind of static flag in the redefined getenv which determines if it's in startup mode (in which case just pass through to the real getenv) or everything's been initialised (in which case it's safe to look it up as an es variable)?

---

Though I'm also on board with just deleting it since HOME autocompletion after you change HOME doesn't seem like a big use case, and rl_reset_terminal will solve the primary problem.

[wryun]

Well, now you've made me actually read the code, I understand that it's already doing something like this. Interesting that it core dumps if called before main. Is that because it implements its own getenv that looks at environ?

[memreflect]

Yes, the esgetenv function was originally named getenv and couldn't be used since the shell was not initialized enough, so it would core dump. The core dump problem was fixed in 0.9-beta1.

However, if the relevant configure test fails to detect an abused getenv as with Debian/OpenBSD, then that means the getenv replacement will never work, even if the configure test was dropped and the replacement getenv was defined unconditionally. If we cannot replace the C library's getenv, then rl_reset_terminal(NULL) will always use the environment's idea of TERM rather than what is defined locally in the shell since we never use setenv or putenv.

As for HOME, LC_ALL, etc., most of them are in the same boat even after readline is initialized:

• HOME: changing this value should affect tilde expansion when using filename completion
• LC_ALL LC_CTYPE LANG: used for character encoding conversion (presumably for things like readline's output-meta option) and NLS
• LS_COLORS: if multiple completions are available, this affects the colors used when listing those completions
• LINES COLUMNS: readline changes these on its own in response to terminal resizing (SIGWINCH)
• INPUTRC:
  • this is currently "safe" for GNU readline since it stores the path of the init file internally after it is read once, so changing this value won't have an effect when rl_re_read_init_file is invoked
  • like TERM, a settor hack could be used to call rl_read_init_file when this value changes, though this may not be a good idea since GNU readline does not expose this function through a command the way re-read-init-file can be bound to a key to call rl_re_read_init_file

[wryun]

Amusingly, readline also uses putenv to set LINES/COLUMNS, so to really support it...

[memreflect]

Yes, we should ideally be able to see the change to LINES and COLUMNS in the shell, which we currently cannot do, and readline should be able to see changes to HOME, TERM, etc. as needed. Basically, the getenv hack needs to be replaced with proper code for changes to the environment. Calling rl_reset_terminal with the shell's idea of TERM would fix things for TERM, but tilde expansion for filename completion would still be a problem since it would use the previous value of HOME, as would colored output when enabled and LS_COLORS is set.

[memreflect]

Well, it seems I was partially mistaken. The abused getenv test is still not required and not entirely working anyway, but all the replacement getenv code can be left as-is aside from removing the conditionals to result in things working properly. While I'm hesitant to rely on this behavior since I'm not fluent in ELF, other shells like Bash replace getenv the same way. Considering Bash's nigh ubiquity, I feel comfortable enough with this despite my personal hesitation.

Also, I want to point out that the stdgetenv function walking the external environ array is what solves the core dump issue, and the realgetenv function pointer is switched to point to the esgetenv function after all of the machinery needed for variables is properly set up. Thinking back, I can only speculate about the reasons why all of that important code was hidden behind the abused getenv test in the first place.

getenv-fix removes the abused getenv test and unconditionally defines the replacement getenv and related code as described above, and when readline wants HOME, TERM, etc., it'll use the replacement getenv function, which will be calling esgetenv by that point. While we could have called setenv/unsetenv or putenv ourselves, it would likely impact performance too much if one used foo = bar or local (fn-foo = @ args { ... }){...} inside of a loop, repeatedly calling setenv and unsetenv on every push and pop. That said, I also added replacements for those functions specifically so readline can set LINES and COLUMNS, allowing the shell to have access to them and any other variables that readline decides to set in the future.

Edit: I have added a few more commits since I first posted this comment. To avoid confusion in the future, this link will always display the changes between the master repo and my getenv-fix branch: Comparing wryun:master...memreflect:getenv-fix

[jpco]

Do you want to make this a PR? It would be nice to be free of ABUSED_GETENV for the sake of simplification, and I think this is basically the Right Thing for any shell to do. My only suggestion would be to get rid of the #if READLINEs and perform the getenv swapout unconditionally, though I respect that's potentially hairy.

Maybe we can take take a page out of bash's book and adapt their BASH_FUNC_GETENV autoconf test?