On Mon, Sep 30, 2019 at 6:29 PM Petr Lautrbach <plautrba@xxxxxxxxxx> wrote: > > Stephen Smalley <sds@xxxxxxxxxxxxx> writes: > > > On 9/26/19 5:58 PM, Nicolas Iooss wrote: > >> On Thu, Sep 26, 2019 at 2:52 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote: > >>> > >>> Fixes: https://github.com/SELinuxProject/selinux/issues/61 > >>> Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx> > >>> --- > >>> python/sepolicy/sepolicy/interface.py | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/python/sepolicy/sepolicy/interface.py b/python/sepolicy/sepolicy/interface.py > >>> index 583091ae18aa..b1b39a492d73 100644 > >>> --- a/python/sepolicy/sepolicy/interface.py > >>> +++ b/python/sepolicy/sepolicy/interface.py > >>> @@ -196,7 +196,7 @@ def get_xml_file(if_file): > >>> from subprocess import getstatusoutput > >>> basedir = os.path.dirname(if_file) + "/" > >>> filename = os.path.basename(if_file).split(".")[0] > >>> - rc, output = getstatusoutput("python /usr/share/selinux/devel/include/support/segenxml.py -w -m %s" % basedir + filename) > >>> + rc, output = getstatusoutput("/usr/bin/python3 /usr/share/selinux/devel/include/support/segenxml.py -w -m %s" % basedir + filename) > >>> if rc != 0: > >>> sys.stderr.write("\n Could not proceed selected interface file.\n") > >>> sys.stderr.write("\n%s" % output) > >> > >> Considering that Python's "command" module was removed in Python 3 > >> (according to https://docs.python.org/2/library/commands.html), and > >> that Python 3's subprocess.getstatusoutput() supports using a list of > >> arguments instead of a string, it seems better to change this code to > >> something like: > > I think this is not correct: > > Execute the string 'cmd' in a shell with 'check_output' and > return a 2-tuple (status, output). The locale encoding is used > to decode the output and process newlines. > > > >>> subprocess.getstatusoutput(["echo", "hey"]) > (0, '') > > >> subprocess.getstatusoutput("echo hey") > (0, 'hey') Indeed, I am so used to subprocess.check_output() and subprocess.Popen(), that can both take arguments as a list, that I expected it to be the same with subprocess.getstatusoutput(), but it is not correct. Sorry for the confusion, and thank you for fixing it! Anyway, using getstatusoutput() by concatenating a path to a command line makes get_xml_file() broken when operating on paths with spaces, as the paths are not quoted nor escaped before they are concatenated. In my humble opinion, I would prefer if the code was written in a more "defensive" way. But because nobody seems to have complained about this so far and because Python's standard library does not help much, I accept keeping getstatusoutput() for now. > >> > >> from subprocess import getstatusoutput > >> basedir = os.path.dirname(if_file) > >> filename = os.path.basename(if_file).split(".")[0] > >> rc, output = getstatusoutput(["python3", > >> "/usr/share/selinux/devel/include/support/segenxml.py", "-w", "-m", > >> os.path.join(basedir, filename)]) > >> > >> The code that I suggest is not compatible with Python 2 (which does > >> not support using list of arguments). Therefore, doing so makes > >> sepolicy really Python-3 only. I do not consider this to be an issue, > >> but others may prefer to wait for 3.0 to be released before dropping > >> support for Python 2 completely. > > > > Anyone else have an opinion on whether we should fix this in a > > python2-compatible manner? > > I'd stay with python2 compatible for now. > > > Also, should it be just "python3" or "/usr/bin/python3"? > > It would be great if it could use $(PYTHON) from Makefile. I agree, but this would be quite complex (the implementations of this idea that I imagine would consists in editing the Python source code with "sed" commands when installing the file). But it would nonetheless be nice if "/usr/share/selinux/devel/include/support/segenxml.py" could also be configured in Makefile... Anyway, for "python3 vs. /usr/bin/python3", I would like to stick as closely as possible with the meaning: use "/usr/bin/..." for system-wide programs/files and use "/usr/bin/env" or "python" for programs that can be run in Python's virtual environments. As /usr/share/selinux/devel/include/support/segenxml.py falls into category "system-wide files", my choice would be for /usr/bin/python3. Thanks, Nicolas
