Author WanderingLogic
Recipients WanderingLogic, akira, asvetlov, chris.jerdonek, christian.heimes, eric.araujo, eryksun, ezio.melotti, gregory.p.smith, lyapun, ned.deily, neologix, pitrou, r.david.murray, serhiy.storchaka
Date 2014-11-06.18:25:13
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1415298314.32.0.0430576457537.issue16353@psf.upfronthosting.co.za>
In-reply-to
Content
In msg230720 Akira Li (akira) wrote:
> os.defpath is supposed to be ':'+CS_PATH, e.g., look at glibc (C library
> used on Linux) sysdeps/posix/spawni.c I don't know whether it is
> possible to change CS_PATH without recompiling every statically linked
> executable on a system, sysdeps/unix/confstr.h:
>
>   #define CS_PATH "/bin:/usr/bin"
>
> Though this issue is about the path to the standard shell sh/cmd.exe.
> It is not about os.defpath.

I understand this issue is about the path to the standard shell.
My argument is that os.defpath is the wrong tool for finding that
path, and os.confstr('CS_PATH') is the correct tool, as you originally
suggested in msg224514.

> The patch [1] has tests. Have you tried to run them?
> The tests *pass* at least on one system ;)
>
> [...]
>
> os.get_shell_executable() does not use cwd. Neither the documentation
> nor the code in the patch suggest that.

I ran the tests (and the entire Python test suite) before I wrote my
first message.  But you didn't test what happens when there is a bogus
'sh' somewhere along the path.  You also have a bug in your path
searching loop that interprets an empty element on the path as "/".
Here's the current failure mode:

Go to root on your Posix system.  With "su" or "sudo" create an
executable file named sh.  (for example "cd /; sudo touch sh; sudo
chmod +x sh").  Then go back to your python interpreter (with the
patch applied) and run "os.get_shell_executable()".  The returned
result will be '/sh'.

The loop _should_ be treating empty path elements as current working
directory.  (In the glibc sysdeps/posix/spawni.c file you pointed to
look around line 281, by the comment that says "/* Two adjacent
colons, or a colon at the beginning or the end of 'PATH' means to
search the current directory.*/")

But if you fix the loop to iterate over the path correctly (including
'cwd') then you will find that putting an executable named 'sh' in the
current working directory will make os.get_shell_executable() return
the "sh" in the current working directory (because os.defpath says it
should).

Note also, that glibc's spawni() uses two different "default paths".
One is for if the user didn't specify their PATH variable (that's the
one where they use ":/bin:/usr/bin") and a completely different (built
in path is used in the spawn*p* version) if it turns out that the file
is a script that needs to run under the system sh.  (On line 290 they
call maybe_script_execute(), which calls script_execute(), which uses
_PATH_BSHELL.)

> os.defpath is the default search path used by exec*p* and spawn*p*
> i.e., if os.defpath is incorrect for these system then os.exec*p* and
> os.spawn*p* functions could also be broken.

I'll look into it.

> I don't remember exactly the motivation to use os.defpath instead of
> os.confstr('CS_PATH').

The motivation was in msg224514.  You wrote:
> In the absense of os.confstr('CS_PATH') on Android (msg175006),
> os.defpath seems appropriate than os.environ['PATH'] to expand 'sh'
> basename to the full path to be used by subprocess later.

But os.defpath doesn't work on Android either (because it is hardcoded
to ':/bin:/usr/bin').
History
Date User Action Args
2014-11-06 18:25:14WanderingLogicsetrecipients: + WanderingLogic, gregory.p.smith, pitrou, christian.heimes, ned.deily, ezio.melotti, eric.araujo, r.david.murray, asvetlov, chris.jerdonek, neologix, akira, serhiy.storchaka, eryksun, lyapun
2014-11-06 18:25:14WanderingLogicsetmessageid: <1415298314.32.0.0430576457537.issue16353@psf.upfronthosting.co.za>
2014-11-06 18:25:14WanderingLogiclinkissue16353 messages
2014-11-06 18:25:13WanderingLogiccreate