On 30/06/13 11:13, Sergio Correia wrote:
From: Sergio Correia <sergio@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/checkforein... - http://ewontfix.com/4/ - http://austingroupbugs.net/view.php?id=529
Signed-off-by: Sergio Correia <sergio@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/checkforein... + * 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) {