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

[Sergio Correia]

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 at archlinux.org> wrote:

> 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.
>
>
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/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) {
> >
>
>
>