0

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

  • What will you do when you want to use both git and say hg at once? Will you have a class deriving from both GitFileWrap and MercurialFilleWrap? – user253751 May 1 '17 at 0:14
  • @immibis I suppose if I wanted to use two vc systems at once for some reason, then yes that is probably what I'll do – ifma May 1 '17 at 3:33
2
  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.
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.

  • I am aware of libgit2 and have tried for almost a month to make it work with my legacy system, but in the end it wasnt worth it (the gcc on the system is too old for example so libgit2 had to be compiled without threading support, which made it very slow). Yes, that is a good suggestion for the base class (use POSIX calls); but if it is not possible to use libgit2 what would be another better solution? I am not the design authority so I have to make the above work... I.e. git operations need to be spawned somehow – ifma Apr 29 '17 at 7:37
  • @ifma - updated with an option to mitigate. – Steve Barnes Apr 29 '17 at 9:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.