[pacman-dev] [PATCH] Looping for EINTR on close() is wrong on Linux
From: Sergio Correia <sergio@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/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) + +/* 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) { -- 1.8.3.1
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) {
Hi, sorry for the delay, I got stuck in some stuff from my day job. On Sat, Jun 29, 2013 at 9:46 PM, Allan McRae <allan@archlinux.org> wrote:
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.
Good question. I am not sure about it, gotta check to make sure. In fact I wasn't even aware we supported such systems :)
This patch changes the CLOSE macro in libalpm not to loop on EINTR any longer. Now it is simply a close() call.
[snip]
@@ -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!
Makes a lot of sense, especially after finding out we support other systems. Thanks for the feedback, Sergio
+/* 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) {
On Sun, Jun 30, 2013 at 11:46:59AM +1000, Allan McRae wrote:
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.
Some quotes:
FreeBSD: It seems pretty clear in FreeBSD that you should not retry the call to close(); sys_close() calls kern_close() which locks the file, frees the file descriptor (!), and /then/ calls closef() (which, if capable of returning EINTR, is already too late).
Mac OS X: Here you should not retry, but you should also be careful; depending on whether you are UNIX2003 you will get different behavior from the close() function from libSystem, directing you to either the syscall close() or close_nocancel().
AIX also closes the fd immediately, while HPUX requires the loop. Idk if we explicitly support either of those two, but it looks like we're safe in killing the close loop. That said, the loop is possibly a source of bugs for another reason, apparently its possible for the fd to have been closed and reused by the time EINTR fires, causing an unrelated fd to be closed.
As discussed in this thread, many POSIX implementations, including Linux, will release the file descriptor immediately, and so if close() fails with EINTR, it is possible that the file descriptor has already been reclaimed and in use by some other component. Retrying the close call, therefore, might close some other component's descriptor.
- https://sites.google.com/site/michaelsafyan/software-engineering/checkforein... - http://linux.derkeiler.com/Mailing-Lists/Kernel/2005-09/3000.html
On Wed, Jul 3, 2013 at 4:38 PM, Simon Gomizelj <simongmzlj@gmail.com> wrote:
That said, the loop is possibly a source of bugs for another reason, apparently its possible for the fd to have been closed and reused by the time EINTR fires, causing an unrelated fd to be closed.
As discussed in this thread, many POSIX implementations, including Linux, will release the file descriptor immediately, and so if close() fails with EINTR, it is possible that the file descriptor has already been reclaimed and in use by some other component. Retrying the close call, therefore, might close some other component's descriptor.
- https://sites.google.com/site/michaelsafyan/software-engineering/checkforein... - http://linux.derkeiler.com/Mailing-Lists/Kernel/2005-09/3000.html
Given that we are single-threaded, I don't see how this is possible. -Dan
Agreed, its just for the record. Especially since there seems to be some conjecture of making pieces of pacman/libalpm multithreaded.
On Wed, Jul 3, 2013 at 4:38 PM, Simon Gomizelj <simongmzlj@gmail.com> wrote:
That said, the loop is possibly a source of bugs for another reason, apparently its possible for the fd to have been closed and reused by the time EINTR fires, causing an unrelated fd to be closed.
As discussed in this thread, many POSIX implementations, including Linux, will release the file descriptor immediately, and so if close() fails with EINTR, it is possible that the file descriptor has already been reclaimed and in use by some other component. Retrying the close call, therefore, might close some other component's descriptor.
- https://sites.google.com/site/michaelsafyan/software-engineering/check foreintrwheninvokingclosethinkagain - http://linux.derkeiler.com/Mailing-Lists/Kernel/2005-09/3000.html
Given that we are single-threaded, I don't see how this is possible.
-Dan
Correct, there is no risk that to have a race condition with another thread that would reuse a fd between 2 successive close() calls. However, in the case close() returns EINTR, calling close() a second will invariably fail so, IMO, there is no point doing it. sys_close does the following: release the process fd table entry atomically (cannot be interrupted, cannot fail for valid fd) flush to disk remaining data still in write buffer. (This part can be interrupted) For those interested, it is pretty simple code in the kernel: fs/open.c close() calls __close_fd fs/file.c __close_fd(): 1. release fdt entry, 2. calls filp_close() fs/open.c filp_close(): Calls filp->f_op->flush and this the only operation that can be interrupted. I do not think that it is a big deal but if you really want to address correctly in regards to EINTR, just call fsync(fd) successfully and loop on it if it returns EINTR before closing the fd. That way, you pretty much guaranty that close() will not return EINTR. ________________________________ CONFIDENTIALITY : This e-mail and any attachments are confidential and may be privileged. If you are not a named recipient, please notify the sender immediately and do not disclose the contents to another person, use it for any purpose or store or copy the information in any medium.
For those interested, it is pretty simple code in the kernel:
fs/open.c close() calls __close_fd fs/file.c __close_fd(): 1. release fdt entry, 2. calls filp_close() fs/open.c filp_close(): Calls filp->f_op->flush and this the only operation that can be interrupted.
I do not think that it is a big deal but if you really want to address correctly in regards to EINTR, just call fsync(fd) successfully and loop on it if it returns EINTR before closing the fd. That way, you pretty much guaranty that close() will not return EINTR.
A small inaccuracy slipped into what I have written flush != sync Only a handful of fs define the file operation flush: exofs nfs cifs ecryptfs fuse For any other fs, close() cannot return EINTR. For the fs that define the flush operation, it seems to be most of the time identical to sync but there is no guaranty that this is 100% accurate and will stay like that in the future. Here is another suggestion: How about blocking signals while closing file? That sounds like the most portable solution but again, personally, I would not care too much about this unlikely event. Just make sure that you do not call close() more than once as it invalidates the fd even when returning an error, IMO that is what is important. ________________________________ CONFIDENTIALITY : This e-mail and any attachments are confidential and may be privileged. If you are not a named recipient, please notify the sender immediately and do not disclose the contents to another person, use it for any purpose or store or copy the information in any medium.
On 04/07/13 08:05, Dan McGee wrote:
On Wed, Jul 3, 2013 at 4:38 PM, Simon Gomizelj <simongmzlj@gmail.com> wrote:
That said, the loop is possibly a source of bugs for another reason, apparently its possible for the fd to have been closed and reused by the time EINTR fires, causing an unrelated fd to be closed.
As discussed in this thread, many POSIX implementations, including Linux, will release the file descriptor immediately, and so if close() fails with EINTR, it is possible that the file descriptor has already been reclaimed and in use by some other component. Retrying the close call, therefore, might close some other component's descriptor.
- https://sites.google.com/site/michaelsafyan/software-engineering/checkforein... - http://linux.derkeiler.com/Mailing-Lists/Kernel/2005-09/3000.html
Given that we are single-threaded, I don't see how this is possible.
This is not possible now... but there is code floating around to set this up and I would like to use it for at least integrity checking. Also, this appears to be the right thing to do whether we are single- or multi-threaded. We should fix the issue whether or not we can expose it. Allan
participants (5)
-
Allan McRae
-
Dan McGee
-
LANGLOIS Olivier PIS -EXT
-
Sergio Correia
-
Simon Gomizelj