[pacman-dev] [PATCH] Use O_CLOEXEC as much as possible when opening files

[Allan McRae]

On 16/07/13 04:32, Dan McGee wrote:
> When calling open(), use O_CLOEXEC as much as possible to ensure the
> file descriptor is closed when and if a process using libalpm forks.
> 
> For most of these cases, and especially in utility functions, the file
> descriptor is opened and closed in the same function, so we don't have
> too much to worry about. However, for things like the log file and
> database lock file, we should ensure descriptors aren't left hanging
> around for children to touch.
> 
> This patch is inspired by the problem in FS#36161, where an open file
> descriptor to the current working directory prevents chroot() from
> working on FreeBSD. We don't need this file descriptor in the child
> process, so open it (and now several others) with O_CLOEXEC.
> 
> Signed-off-by: Dan McGee <dan at archlinux.org>
> ---

Compile error:

  CC       libalpm_la-log.lo
In file included from /usr/include/fcntl.h:296:0,
                 from util.h:41,
                 from <command-line>:27:
In function 'open',
    inlined from 'alpm_logaction' at <command-line>:52:3:
/usr/include/bits/fcntl2.h:50:24: error: call to '__open_missing_mode'
declared with attribute error: open with O_CREAT in second argument
needs 3 arguments
    __open_missing_mode ();
                        ^



>  lib/libalpm/add.c    |  2 +-
>  lib/libalpm/handle.c |  2 +-
>  lib/libalpm/log.c    |  8 ++++++--
>  lib/libalpm/util.c   | 14 +++++++-------
>  4 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
> index 8ef9ef0..2d93077 100644
> --- a/lib/libalpm/add.c
> +++ b/lib/libalpm/add.c
> @@ -542,7 +542,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg,
>  		}
>  
>  		/* save the cwd so we can restore it later */
> -		OPEN(cwdfd, ".", O_RDONLY);
> +		OPEN(cwdfd, ".", O_RDONLY | O_CLOEXEC);
>  		if(cwdfd < 0) {
>  			_alpm_log(handle, ALPM_LOG_ERROR, _("could not get current working directory\n"));
>  		}
> diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c
> index 53c86c5..274e3bc 100644
> --- a/lib/libalpm/handle.c
> +++ b/lib/libalpm/handle.c
> @@ -110,7 +110,7 @@ int _alpm_handle_lock(alpm_handle_t *handle)
>  	FREE(dir);
>  
>  	do {
> -		fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000);
> +		fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL | O_BINARY | O_CLOEXEC, 0000);
>  	} while(fd == -1 && errno == EINTR);
>  	if(fd >= 0) {
>  		FILE *f = fdopen(fd, "w");
> diff --git a/lib/libalpm/log.c b/lib/libalpm/log.c
> index 271bd00..5f3b094 100644
> --- a/lib/libalpm/log.c
> +++ b/lib/libalpm/log.c
> @@ -49,9 +49,13 @@ int SYMEXPORT alpm_logaction(alpm_handle_t *handle, const char *prefix,
>  
>  	/* check if the logstream is open already, opening it if needed */
>  	if(handle->logstream == NULL) {
> -		handle->logstream = fopen(handle->logfile, "a");
> +		int fd;
> +		OPEN(fd, handle->logfile, O_WRONLY | O_APPEND | O_CREAT | O_CLOEXEC);
> +		if(fd >= 0) {
> +			handle->logstream = fdopen(fd, "a");
> +		}
>  		/* if we couldn't open it, we have an issue */
> -		if(handle->logstream == NULL) {
> +		if(fd < 0 || handle->logstream == NULL) {
>  			if(errno == EACCES) {
>  				handle->pm_errno = ALPM_ERR_BADPERMS;
>  			} else if(errno == ENOENT) {
> diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
> index 1e21362..863614e 100644
> --- a/lib/libalpm/util.c
> +++ b/lib/libalpm/util.c
> @@ -154,9 +154,9 @@ int _alpm_copyfile(const char *src, const char *dest)
>  
>  	MALLOC(buf, (size_t)ALPM_BUFFER_SIZE, return 1);
>  
> -	OPEN(in, src, O_RDONLY);
> +	OPEN(in, src, O_RDONLY | O_CLOEXEC);
>  	do {
> -		out = open(dest, O_WRONLY | O_CREAT, 0000);
> +		out = open(dest, O_WRONLY | O_CREAT | O_BINARY | O_CLOEXEC, 0000);
>  	} while(out == -1 && errno == EINTR);
>  	if(in < 0 || out < 0) {
>  		goto cleanup;
> @@ -245,7 +245,7 @@ int _alpm_open_archive(alpm_handle_t *handle, const char *path,
>  	archive_read_support_format_all(*archive);
>  
>  	_alpm_log(handle, ALPM_LOG_DEBUG, "opening archive %s\n", path);
> -	OPEN(fd, path, O_RDONLY);
> +	OPEN(fd, path, O_RDONLY | O_CLOEXEC);
>  	if(fd < 0) {
>  		_alpm_log(handle, ALPM_LOG_ERROR,
>  				_("could not open file %s: %s\n"), path, strerror(errno));
> @@ -327,7 +327,7 @@ int _alpm_unpack(alpm_handle_t *handle, const char *path, const char *prefix,
>  	oldmask = umask(0022);
>  
>  	/* save the cwd so we can restore it later */
> -	OPEN(cwdfd, ".", O_RDONLY);
> +	OPEN(cwdfd, ".", O_RDONLY | O_CLOEXEC);
>  	if(cwdfd < 0) {
>  		_alpm_log(handle, ALPM_LOG_ERROR, _("could not get current working directory\n"));
>  	}
> @@ -503,7 +503,7 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[])
>  	int retval = 0;
>  
>  	/* save the cwd so we can restore it later */
> -	OPEN(cwdfd, ".", O_RDONLY);
> +	OPEN(cwdfd, ".", O_RDONLY | O_CLOEXEC);
>  	if(cwdfd < 0) {
>  		_alpm_log(handle, ALPM_LOG_ERROR, _("could not get current working directory\n"));
>  	}
> @@ -769,7 +769,7 @@ static int md5_file(const char *path, unsigned char output[16])
>  
>  	MALLOC(buf, (size_t)ALPM_BUFFER_SIZE, return 1);
>  
> -	OPEN(fd, path, O_RDONLY);
> +	OPEN(fd, path, O_RDONLY | O_CLOEXEC);
>  	if(fd < 0) {
>  		free(buf);
>  		return 1;
> @@ -811,7 +811,7 @@ static int sha2_file(const char *path, unsigned char output[32], int is224)
>  
>  	MALLOC(buf, (size_t)ALPM_BUFFER_SIZE, return 1);
>  
> -	OPEN(fd, path, O_RDONLY);
> +	OPEN(fd, path, O_RDONLY | O_CLOEXEC);
>  	if(fd < 0) {
>  		free(buf);
>  		return 1;
>