Fix encoding issue when using subprocess in a2x

[MasterOdin]

This was a bug I encountered when running the make process (and it was compiling a2x man page). subprocess.Popen returns binary strings in Python3. We need to either manually decode them or use universal_newlines to get them into unicode strings.

Once Python 3.4 is dropped, it'd probably make sense to replace this call with using subprocess.run.

[elextr]

Since AFAICT the only place that the returns of shell() are used is to get the backend list from the asciidoc command we should be able to control the contents, so this should be fine.

A side note on shell(), somebody with windows should check if the if os == NT: part still works or is needed.

[MasterOdin]

We can remove it so long as the python file that is linked has a shebang line.

See:

• https://docs.python.org/3/using/windows.html#getting-started
• https://docs.python.org/3/using/windows.html#shebang-lines

May have been a slightly different story pre-3.3, but for 3.4+ (which asciidoc-py3 targets), we should be in the clear for "standard" python installations.

Not sure how worth the quotation thing is leaving in given that Windows XP+ gives commands a max length of 8191 characters (though this is significantly less than the limit on unix-based systems (macOS has a limit of 262144 characters for example).

[elextr]

As I read your references you still need to use the launcher, ie py on windows because the OS itself does not support shebangs. So if the command is foo.py on *x then it needs to be py foo.py on windows.

[MasterOdin]

Looking back over them, yeah, it's not actually clear that you don't have to use the launcher (or python3.exe). The launcher (via the general python installer) should set up your file associations for py files such that .py files will automatically be handled by the PyLauncher which will process the shebang and use the proper version of Python (Python3), assuming they've installed a version of python3 that asciidoc-py3 supports and they didn't decide to turn off settings like the file association. If you want to fully support all users even those without file associations, then leaving the executable (though probably changing it to py) is necessary.

[elextr]

For the quoting, I also do stuff on the Geany IDE and spawning processes on Windows is a real pain, in particular quoting seems to be variable, arcane, and version specific. 😠

My personal feeling is that its only necessary to support windows versions that are still in mainstream support for the consumer version, unless someone particularly contributes any needed support for an older version. (If Microsoft isn't supporting you with bugfixes, or you are a business, why do you expect a volunteer project to do so without your contribution?).

I think since January that means Windows 10 only. Not sure if that makes it easier.

[MasterOdin]

Realistically, I think it'd be more important to fix the calls to the shell function such that we set shell=False in the subprocess call and avoid a lot of this messiness with quoting as we're not utilizing any shell specific features to also have to justify the increased security threat of shell=True.

And given that Windows 10 is the only operating system I've got easy access to, it does make testing easier to only worry about Windows 10 for now, though if it works on Windows 10, chances are good it'll work for 8 as well.

I'll write up an issue though to track the two avenues of discussion: 1. check windows support and remove the stuff above subprocess if no longer needed, 2. fix a2x to not require shell=True.

[elextr]

Ok.

Given that Asciidoc itself spawns filters and other stuff, I'm not sure the shell injection insecurities are such a big problem, but one solution (possibly to both quoting and needing shell) might be to pass cmd as a list instead of a string. Then quoting (which is used on executable names in a2x, I assume to allow for toolchains installed in users home directories with spaces) won't be needed, and so shell won't be needed (so long as no a2x backends use it).

[MasterOdin]

I've opened #24 for the discussion about shell=True (with a proof of concept of an attack vector that exists within asciidoc because of it).

To continue the windows discussion, and tested some stuff on my Windows 10 machine.

I used the following file for the test: test.py

#!/usr/bin/env python3
print("Hello world")

The computer had 3.5 installed from a year and half ago, and typing just test.py in cmd.exe threw an error about unknown file association. I uninstalled 3.5 and installed the newest 3.6 on default settings. Typing just test.py in cmd.exe worked fine then. For broad support, it's probably worth just leaving it in place that we use the sys.executable. It might be worth setting this for all systems though given that theoretically, you could be executing a2x.py via python3.4 while python3 points to python3.6 and therefore you'd probably not want to asciidoc to be executed via python3.6 (which is what would happen using the shebang.

However, I think all of this is probably outside of the stuff to be done in this PR and this discussion should probably be moved to a relevant issue?