Shell completely broken on platforms without `O_CLOEXEC`

[zhiayang]

Fish version: 3.2.2, built from source. Platform: Solaris 10, SPARC (64-bit).

After compiling fish on such a platform (where O_CLOEXEC doesn't exist), running it gives the following output:

zhiayang@sunfire0:~/code/fish-3.2.2/build[1022]$ ./fish
Unable to open the current working directory: Error 0
source: Error encountered while sourcing file '/user/z/zhiayang/code/fish-3.2.2/share/config.fish':
source: Error 0
source: Error encountered while sourcing file '/user/z/zhiayang/code/fish-3.2.2/etc/config.fish':
source: Error 0
zhiayang@<hostname> /home/z/zhiayang/code/fish-3.2.2/build > error: Unable to open a pipe for universal variables using '/var/tmp//fish.zhiayang/fish_universal_variables.notifier': File exists

Clearly, everything is broken, backspacing doesn't work, etc. The files stated actually exist (of course -- the current directory must also exist). What got me investigating was the fact that we have Error 0, which means no error.

After some printf debugging and digging around, the culprit is here, at fds.cpp:254:

#ifdef O_CLOEXEC
    fd = open(path, flags | O_CLOEXEC, mode);
#else
    fd = open(path, flags, mode);
    if (fd >= 0 && !set_cloexec(fd)) {
        exec_close(fd);
        fd = -1;
    }
#endif

After looking at the return value of set_cloexec, it is very obvious that this code path has never been tested and run. set_cloexec() returns non-zero on error... which causes this function to fail -- when it should have succeeded.

The correct comparison should be set_cloexec(fd) != 0.

[faho]

it is very obvious that this code path has never been tested and run

Yup, because most things have O_CLOEXEC and we have no CI on Solaris and barely any users. Sorry!

Really, our preferred solution would be for every platform to just acquire O_CLOEXEC.

set_cloexec(fd) != 0

So you mean this should be

    if (fd >= 0 && set_cloexec(fd)) {

?

I would appreciate it if you would open a PR with the patch, because otherwise we'll end up describing the code instead of just writing it, and since I have no platform without O_CLOEXEC to test on (and no SPARC machine at all), I can't be sure that I've interpreted what you wrote correctly.

EDIT: I've now applied my best reading of what you wrote. In future, if you already know the solution, please open a PR.

[zhiayang]

Ah, thanks so much for responding so quickly. Thankfully this wasn't too hard to pin down. 👍🏻

EDIT: i should say that that condition also works. Understandably this went under the radar on an obscure platform (:

[faho]

I should state for those reading along at home that O_CLOEXEC was apparently added in Solaris 11 in... 2011.

Even if we added Solaris CI (and it would probably be Illumos rather than Oracle Solaris) we would never have added Solaris 10.

[zhiayang]

Ah yes, Solaris 10 is definitely very very out of date.

[zanchey]

@zhiayang I would be interested to know if the test suite passes now that interactive mode is working.

[zhiayang]

@zanchey Most of the tests pass, barring the something about piping universal variables with strategy 2, and some dropped history items. Anyway from just using the shell, nothing seems broken so far.