Wrapping a shell process to manipulate a file system, how to improve design?

Question

In my API library (C++) I am wrapping a shell process (spawned using popen) with several classes to manipulate a file system.

The base class ProcessFileWrap wraps general filesystem tasks and executes other custom tasks from its derived classes. It looks like below in pseudocode:

// ProcessFileWrap base class methods

ExecCustomProcess(string process) // does popen(process, "r")

ChangeDir(string path) // changes the current directory

AddFolder(string path) // wrapper for mkdir

AddFile(string path) // wrapper for touch

MakeLink(string pathToFile, string pathToLink) // wrapper for ln -s

// ... And so on

There are several derived classes. One of them is GitFileWrap, which wraps git processes like so (again in psuedocode). It needs to do some specific file system tasks after or during the process:

// GitFileWrap derived class methods

// Calls ChangeDir(pathToRepo) and ExecCustomProcess("git pull")
// Afterwards it creates links for each pulled file in a custom 
// directory via MakeLink(file, customLoc).
Pull()

// Calls ExecCustomProcess(git commit -m obj.GetCommitMessage())
// A GitObject represents a git versioned object in the filesystem.
Commit(GitObject obj)

// ... And so on

So users of the API would call methods from the derived classes as needed.

I am aware that wrapping processes is in general not best practice, so if you can, please comment on the design with the assumption that there isn't really another alternative (or I guess you could, but I would appreciate comments on the design above). Thanks!

Note: It is not possible for me to use libgit2

Answer 1

1. Don't use external processes if you can help it. Use the system calls directly: chdir(path), mkdir(pathname, mode), std::fstream or open(path, O_CREAT) instead of touch, symlink(target, path) instead of ln -s. In a C++ program, there are just no good reasons to use the wrapper binaries instead of just calling these syscalls directly.
2. When you really have to use external commands, don't use popen(). popen() is slow and hopelessly insecure because it always starts a shell and therefore it also needlessly interprets globs and shell variables. The safe way to start a subprocess is with fork() and execve() or posix_spawn(). These functions passes the program files and parameters as an array instead of requiring you to interpolate and escape your arguments into a strings, only to have a shell interpreter try to parse the command arguments to figure out how to split your strings that it eventually passes to execve anyway.

Answer 2

Problems with the design above:

1. It will be slow, due to the need to spawn a new process for each operation and wait for it to finish.
2. If you try to address the above by not waiting for spawned processes to finish it will be vulnerable to race conditions - e.g. spawn a task that creates a directory & then one to touch files in that directory there is a risk that the directory does not exist yet.
3. It will be a security nightmare - unless you do extensive checking there is a strong risk that malicious or impatient individuals will make use of the fact that you are spawning e.g. ChangeDir("/ && rm -rf .") and many variants.
4. Errors will be indicated by the return code but for more details on what was wrong you will need to parse the stdout: &/or stderr: streams.

Accordingly my comment on the design that is detailed in the question is a resounding don't even think about doing it that way! - If there is literally no other other choice - see below for alternatives - then you will need to spend a huge amount of time and effort on parsing input values to make sure that they are safe and there will still be problems.

Possible solutions

1. There are already POSIX C &/or C++ library calls to do just about all of the above OS operations, most of which have clear error codes for what any problem was. e.g. #include <unistd.h> gives you access to: int chdir(const char *path);

2. For git operations use a git library such as libgit2

Basically you just need to spend a little time reading the documentation and/or searching the web to be able to do everything that you describe in a safe, fast, compact manner.

If you are unable to use libraries for specific utilities such as git there is still no excuse to use spawn for things that are in the generic C & C++ libraries.

To mitigate the speed, race and security issues I would recommend collecting, (and thoroughly checking), arguments such as commit messages and creating one off, temporary, shell scripts so that you can launch a single shell executing the sequence of commands, e.g. for git. You can also look at chaining git commands in such scripts.