| Message ID | 20190926125218.22958-1-sds@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | python/sepolicy: call segenxml.py with python3 | expand |
On Thu, Sep 26, 2019 at 2:52 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > > Fixes: https://github.com/SELinuxProject/selinux/issues/61 > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > 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: 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. By the way, the current code is quite misleading because ("%s" % a + b) is interpreted as (("%s" % a) + b), not ("%s" % (a + b)). Thankfully the "%s" is at the end of the format string, but if you want to keep Python 2 compatibility, I suggest adding parentheses somewhere. Nicolas
On 9/26/19 5:58 PM, Nicolas Iooss wrote: > On Thu, Sep 26, 2019 at 2:52 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: >> >> Fixes: https://github.com/SELinuxProject/selinux/issues/61 >> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >> --- >> 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: > > 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? Also, should it be just "python3" or "/usr/bin/python3"? > > By the way, the current code is quite misleading because ("%s" % a + > b) is interpreted as (("%s" % a) + b), not ("%s" % (a + b)). > Thankfully the "%s" is at the end of the format string, but if you > want to keep Python 2 compatibility, I suggest adding parentheses > somewhere.
Stephen Smalley <sds@tycho.nsa.gov> writes: > On 9/26/19 5:58 PM, Nicolas Iooss wrote: >> On Thu, Sep 26, 2019 at 2:52 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: >>> >>> Fixes: https://github.com/SELinuxProject/selinux/issues/61 >>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >>> --- >>> 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') >> >> 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. >> >> By the way, the current code is quite misleading because ("%s" % a + >> b) is interpreted as (("%s" % a) + b), not ("%s" % (a + b)). >> Thankfully the "%s" is at the end of the format string, but if you >> want to keep Python 2 compatibility, I suggest adding parentheses >> somewhere.
On Mon, Sep 30, 2019 at 6:29 PM Petr Lautrbach <plautrba@redhat.com> wrote: > > Stephen Smalley <sds@tycho.nsa.gov> writes: > > > On 9/26/19 5:58 PM, Nicolas Iooss wrote: > >> On Thu, Sep 26, 2019 at 2:52 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > >>> > >>> Fixes: https://github.com/SELinuxProject/selinux/issues/61 > >>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> > >>> --- > >>> 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
On 9/30/19 3:36 PM, Nicolas Iooss wrote: > On Mon, Sep 30, 2019 at 6:29 PM Petr Lautrbach <plautrba@redhat.com> wrote: >> >> Stephen Smalley <sds@tycho.nsa.gov> writes: >> >>> On 9/26/19 5:58 PM, Nicolas Iooss wrote: >>>> On Thu, Sep 26, 2019 at 2:52 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: >>>>> >>>>> Fixes: https://github.com/SELinuxProject/selinux/issues/61 >>>>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >>>>> --- >>>>> 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. So, are people ok with merging the patch as posted in order to fix the open issue before the next release?
On Mon, Oct 7, 2019 at 3:23 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On 9/30/19 3:36 PM, Nicolas Iooss wrote: > > On Mon, Sep 30, 2019 at 6:29 PM Petr Lautrbach <plautrba@redhat.com> wrote: > >> > >> Stephen Smalley <sds@tycho.nsa.gov> writes: > >> > >>> On 9/26/19 5:58 PM, Nicolas Iooss wrote: > >>>> On Thu, Sep 26, 2019 at 2:52 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > >>>>> > >>>>> Fixes: https://github.com/SELinuxProject/selinux/issues/61 > >>>>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> > >>>>> --- > >>>>> 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. > > So, are people ok with merging the patch as posted in order to fix the > open issue before the next release? Yes, even though it would be nicer if the parameters were surrounded by parentheses: rc, output = getstatusoutput("/usr/bin/python3 /usr/share/selinux/devel/include/support/segenxml.py -w -m %s" % (basedir + filename)) Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org> Nicolas
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)
Fixes: https://github.com/SELinuxProject/selinux/issues/61 Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> --- python/sepolicy/sepolicy/interface.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)