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

[Sergio Correia]

From: Sergio Correia <sergio at correia.cc>

Linux closes the descriptor unconditionally even if the close() call is
interrupted.

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)
+
+/* 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) {
-- 
1.8.3.1