Opened 6 years ago

Closed 6 years ago

#287 closed defect (fixed)

Prioritize SHELL over EMXSHELL

Reported by: dmik Owned by: bird
Priority: normal Milestone: libc-0.6.6
Component: libc Version: 0.6.5
Severity: normal Keywords:


The EMXSHELL is used to override COMSPEC for popen() and system() calls in kLIBC. The order of checking in the first case is EMXSHELL, COMSPEC and in the second — EMXSHELL, COMSPEC, OS2_SHELL, SHELL.

At the same time, many scripts in the unix environment (e.g. configure scripts) use SHELL to override the shell command to use. However, given that in the first case SHELL isn't used at all and in the second case it's checked after EMXSHELL and COMSPEC, even if the script sets SHELL (e.g. to point to sh.exe), kLIBC will not notice this and will still use the value of COMSPEC (which is usually cmd.exe). Not quite what the scripts expect.

I think it makes sense to check for SHELL before checking for EMXSHELL or COMSPEC. The order should be the same in both calls: SHELL, EMXSHELL, COMSPEC, OS2_SHELL.

The patch is trivial (in files lib/io/popen.c and lib/process/system.c) so not attaching it here.

Attachments (1)

system.diff (644 bytes) - added by KO Myung-Hun 6 years ago.
patch for system()

Download all attachments as: .zip

Change History (10)

comment:1 Changed 6 years ago by dmik

BTW, if it's changed as suggested, then should also be changed accordingly (search for EMXSHELL there).

Last edited 6 years ago by dmik (previous) (diff)

comment:2 Changed 6 years ago by bird

popen and system shouldn't consult any environment variables at all, it's hardcoded to /bin/sh normally. So, configure script don't expect system or popen to change behavior when they modify SHELL. Since we don't have any standard bourne shell on OS/2 by default, having EMXSHELL as pointer to it sounds good to me. Consulting SHELL before COMSPEC & OS2_SHELL would make sense to unixy processes, I guess.

I'll be changing the two calls to consider the same variables and in the same order: EMXSHELL, SHELL, COMSPEC, and OS2_SHELL. I'll also leave a todo for using /@unixroot/bin/sh[.exe] later, only this needs to only happen to wannabe-unix processes.

See also:

comment:3 Changed 6 years ago by bird

r3857 and r3858 on trunk (untested).

comment:4 Changed 6 years ago by bird

Milestone: libc-0.7libc-0.6.6
Owner: set to bird
Status: newassigned

comment:5 Changed 6 years ago by bird

Resolution: fixed
Status: assignedclosed

Backported in r3859.

comment:6 Changed 6 years ago by dmik

Nice, thanks!

comment:7 Changed 6 years ago by KO Myung-Hun

r3857 and r3859 introduced a regression. system() always falls into an interactive shell mode. An empty string test for pszCmdLine should be negated.

And if there is /@unixroot/bin/sh.exe, then system() cannot execute OS/2 builtin commands such as copy and del. How about using just env vars instead of checking a hardcoded path first ?

comment:8 Changed 6 years ago by KO Myung-Hun

Resolution: fixed
Status: closedreopened

Changed 6 years ago by KO Myung-Hun

Attachment: system.diff added

patch for system()

comment:9 Changed 6 years ago by bird

Resolution: fixed
Status: reopenedclosed

Thanks for noticing. Fixed in r3909.

Note: See TracTickets for help on using tickets.