[pacman-dev] [PATCH] Looping for EINTR on close() is wrong on Linux

[Allan McRae]

On 30/06/13 11:13, Sergio Correia wrote:
> From: Sergio Correia <sergio at correia.cc>
> 
> Linux closes the descriptor unconditionally even if the close() call is
> interrupted.

Is this also the case for other ELF using systems we support (BSD,
HURD)?  I believe it is but I am being too lazy to check.

> This patch changes the CLOSE macro in libalpm not to loop on EINTR any
> longer. Now it is simply a close() call.
> 
> There was another site performing such a loop on EINTR after close, in
> pacman/conf.c, and it was also changed to a simple close() call.
> 
> Links for reference:
> - http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html
> - http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR
> - https://sites.google.com/site/michaelsafyan/software-engineering/checkforeintrwheninvokingclosethinkagain
> - http://ewontfix.com/4/
> - http://austingroupbugs.net/view.php?id=529
> 
> Signed-off-by: Sergio Correia <sergio at correia.cc>
> ---
>  lib/libalpm/util.h | 10 +++++++++-
>  src/pacman/conf.c  |  7 +++----
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h
> index 24b7c22..09834af 100644
> --- a/lib/libalpm/util.h
> +++ b/lib/libalpm/util.h
> @@ -88,7 +88,15 @@ void _alpm_alloc_fail(size_t size);
>  #endif
>  
>  #define OPEN(fd, path, flags) do { fd = open(path, flags | O_BINARY); } while(fd == -1 && errno == EINTR)
> -#define CLOSE(fd) do { int _ret; do { _ret = close(fd); } while(_ret == -1 && errno == EINTR); } while(0)
> +

Comments should not be Linux specific.  Also, that comment is beyond
excessive!

> +/* Linux closes the file descriptor unconditionally, even if it returns EINTR, and hence looping for such
> + * condition after calling close() is wrong and can be potentially dangerous, especially in multithreaded code.
> + * http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html
> + * http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR
> + * https://sites.google.com/site/michaelsafyan/software-engineering/checkforeintrwheninvokingclosethinkagain
> + * http://ewontfix.com/4/
> + * http://austingroupbugs.net/view.php?id=529 */
> +#define CLOSE(fd) do { close(fd); } while(0)
>  
>  /**
>   * Used as a buffer/state holder for _alpm_archive_fgets().
> diff --git a/src/pacman/conf.c b/src/pacman/conf.c
> index 5adc96c..8a42306 100644
> --- a/src/pacman/conf.c
> +++ b/src/pacman/conf.c
> @@ -257,14 +257,13 @@ static int download_with_xfercommand(const char *url, const char *localpath,
>  cleanup:
>  	/* restore the old cwd if we have it */
>  	if(cwdfd >= 0) {
> -		int close_ret;
>  		if(fchdir(cwdfd) != 0) {
>  			pm_printf(ALPM_LOG_ERROR, _("could not restore working directory (%s)\n"),
>  					strerror(errno));
>  		}
> -		do {
> -			close_ret = close(cwdfd);
> -		} while(close_ret == -1 && errno == EINTR);
> +		/* looping for EINTR on close() is wrong on Linux, as it closes the
> +		 * descriptor unconditionally even if the close() call is interrupted.*/
> +		close(cwdfd);
>  	}
>  
>  	if(ret == -1) {
>