proposed popen replacement using posix_spawn

proposed popen replacement using posix_spawn

[Rich Felker]

Hi all,

I've rewritten popen using posix_spawn and, based on minimal testing,
it seems to be working. Please take a look and let me know if you see
any issues with it; if I don't hear any problems soon I'll probably
commit and fix bugs later if necessary. The important corner cases
seem to be:

- fd exhaustion
- memory exhaustion
- cases where stdio and/or stdout is closed prior to popen

Rich

Re: proposed popen replacement using posix_spawn

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 266 bytes --]

On Sat, Mar 23, 2013 at 09:40:17PM -0400, Rich Felker wrote:
> Hi all,
> 
> I've rewritten popen using posix_spawn and, based on minimal testing,
> it seems to be working. Please take a look and let me know if you see

And here's the forgotten attachment. :-)

Rich

[-- Attachment #2: popen.c --]
[-- Type: text/plain, Size: 1187 bytes --]

#include <fcntl.h>
#include <unistd.h>
#include <errno.h>
#include <spawn.h>
#include "stdio_impl.h"
#include "syscall.h"

extern char **__environ;

FILE *popen(const char *cmd, const char *mode)
{
	int p[2], op, e;
	pid_t pid;
	FILE *f;
	posix_spawn_file_actions_t fa;

	if (*mode == 'r') {
		op = 0;
	} else if (*mode == 'w') {
		op = 1;
	} else {
		errno = EINVAL;
		return 0;
	}
	
	if (pipe2(p, O_CLOEXEC)) return NULL;
	f = fdopen(p[op], mode);
	if (!f) {
		__syscall(SYS_close, p[0]);
		__syscall(SYS_close, p[1]);
		return NULL;
	}
	FLOCK(f);

	/* Remove close-on-exec flag if dup2 will be a no-op in the child */
	if (p[1-op] == 1-op) fcntl(1-op, F_SETFD, 0);

	e = ENOMEM;
	if (!posix_spawn_file_actions_init(&fa)) {
		if (!posix_spawn_file_actions_adddup2(&fa, p[1-op], 1-op)) {
			if (!(e = posix_spawn(&pid, "/bin/sh", &fa, 0,
			    (char *[]){ "sh", "-c", (char *)cmd, 0 }, __environ))) {
				posix_spawn_file_actions_destroy(&fa);
				f->pipe_pid = pid;
				fcntl(p[op], F_SETFD, 0);
				__syscall(SYS_close, p[1-op]);
				FUNLOCK(f);
				return f;
			}
		}
		posix_spawn_file_actions_destroy(&fa);
	}
	fclose(f);
	__syscall(SYS_close, p[1-op]);

	errno = e;
	return 0;
}

Re: proposed popen replacement using posix_spawn

[Rich Felker]

On Sat, Mar 23, 2013 at 09:40:57PM -0400, Rich Felker wrote:
> 	/* Remove close-on-exec flag if dup2 will be a no-op in the child */
> 	if (p[1-op] == 1-op) fcntl(1-op, F_SETFD, 0);

This part is wrong; it creates a file descriptor leak race condition
in the parent. The only solution I can see is that the parent must
ensure p[1-op]!=1-op before calling posix_spawn. The easiest way to do
that is to move p[1-op] to a new fd number using F_DUPFD_CLOEXEC when
this happens, and fail the entire operation if this fails. I'll update
the code accordingly.

Rich