Re: [boost] [process] Formal Review starts today, 27 October

Subject: Re: [boost] [process] Formal Review starts today, 27 October
From: Tom Kent (lists_at_[hidden])
Date: 2016-10-27 20:12:28

On Thu, Oct 27, 2016 at 1:26 AM, Antony Polukhin <antoshkka_at_[hidden]>
wrote:

>

> Dear Boost community,
>
> The formal review of Klemens David Morgenstern's Process library
> begins today, 27th October and ends on 5th November.
>
> We encourage your participation in this review. At a minimum, kindly
state:
> - Whether you believe the library should be accepted into Boost
>   * Conditions for acceptance

I believe this is borderline, with some serious quality issues, but should
be accepted.
With the following conditions:
* Documentation is cleaned up substantially (see issues and pull request I
submitted in github)
* The documentation indicates that "There is no guarantee in which order
the arguments will be applied!". This either needs to be fixed so that the
order is always guaranteed or it needs to be greatly elaborated on as to
when the order is kept. Process argument order is very often important.
* There needs to be a warning (or at least explanation of the implications
of or resilience to) user-input going to command line calls with relation
to shell injection vulnerabilities[1]. The python subprocess module has a
good warning message we could probably borrow [2] if necessary.
* Needs to specify setup (explain that this is a header-only library) and
dependencies (the basic example needs boost::filesystem, even though it
just uses strings?).
* Need to fix introduction/tutorial examples (probably the library
implementation) so they run on windows (and use the path to find exes).

>
> - Your knowledge of the problem domain

Medium, from a user perspective. I've worked extensively with Python's
similar Subprocess module.

>
>
> You are strongly encouraged to also provide additional information:
> - What is your evaluation of the library's:
>   * Design

I like the design very much. The one big issue that I'm unsure about is
that it is difficult to break out of a loop based on a pstream when the
program exits. Maybe some EOF signal needed?

I like the basic system/spawn/child design.

Though I didn't dig too deep, the Sync/Async IO seems good.

It seems weird that terminating a process doesn't terminate its children by
default and you need to use a group. It seems like that should be an option
of the child object (defaulted to on).

Environment is OK, is there a reason this_process isn't in the process
namespace?

>   * Implementation

>From what I've seen running on windows, there are some prominent holes in
the implementation. These seem to be bugs that could be fixed quickly.

>
>   * Documentation

Needs lots of help. Could also use more complex examples. I'm also left
with the suspicion that there is more to the library than is covered by the
docs. Some of the things mentioned in the tutorial seem to have other ways
to use them, these should be called out in specific documentation for the
items.

>
>   * Tests

I didn't actually look at the tests, but was disappointed to see that there
was only 44% coverage.

>
>   * Usefulness

Yes! This is something that definitely needs to be in boost.

>
> - Did you attempt to use the library? If so:
>   * Which compiler(s)

>
>   * What was the experience? Any problems?

msvc-12.0 - Visual Studio 2013 - Test suite failed.
msvc-14.0 - Visual Studio 2015

I ran into issues with some pretty basic examples.

        boost::filesystem::path p = "python.exe";
        std::system("python.exe --version"); // Works
        bp::system("python.exe --version"); // Works
        bp::system("python.exe", "--version"); // Fails
        bp::system(p, "--version");  // Fails
        bp::system("C:\\Python27-32\\python.exe", "--version"); // Works

It seems that the library isn't correctly working with the path on windows,
when the arguments are split from the executable.

> - How much effort did you put into your evaluation of the review?

>

A few hours.

I hope these issues can get resolved and this can be included with boost!
Tom

[1] https://en.wikipedia.org/wiki/Shell_injection#Shell_injection
[2]
https://docs.python.org/3/library/subprocess.html#security-considerations