Re: [PATCH] history locking with fcntl

[Vincent Lefevre <vincent@xxxxxxxxxx>]

On 2008-05-04 18:25:51 -0700, Wayne Davison wrote:
> On Mon, Apr 21, 2008 at 03:19:37PM +0200, Vincent Lefevre wrote:
> > Note that since out has been closed, tmpfile is no longer locked.
> > The target file isn't locked either.
> 
> The target file was locked prior to the rename (since that's the only
> file my code is locking -- the HISTFILE itself).  So, the rename does
> essentially unlock the HISTFILE as far as fcntl() is concerned.

I've looked again at the current zsh code (which corresponds to your
patch). This is incorrect.

As I've said, there's a fclose() that has the effect to unlock the file:

        if (fclose(out) < 0 && ret >= 0)
            ret = -1;

Indeed, out is a stream associated with fd, which corresponds to the
history file in general (there's also a case of a tmp file, let's
forget this one). And according to the Linux fclose() man page, the
fclose() function closes the underlying file descriptor:

    The fclose() function will flush the stream pointed to by fp
    (writing any buffered output data using fflush(3)) and close
    the underlying file descriptor.

As a file descriptor associated with the file has been closed, the
file is no longer locked by the process, according to the Linux
fcntl(2) man page:

    As well as being removed by an explicit F_UNLCK, record locks
    are automatically released when the process terminates or if
    it closes _any_ file descriptor referring to a file on which
    locks are held.

So, there's a possible race condition, in particular knowning the
fact that preemption at system calls are more likely to occur. If
  1. the kernel preempts the process (A) at the end of the close()
     [the system call used by fclose()] or just before the rename(),
  2. another process (B) locks the history file,
  3. B is preempted,
  4. (A) does the rename,
then bad things can happen.

In my patch, I did a fflush() instead of fclose() to avoid removing
the lock at this time, and I did the fclose() after the rename().

-- 
Vincent Lefèvre <vincent@xxxxxxxxxx> - Web: <http://www.vinc17.org/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.org/blog/>
Work: CR INRIA - computer arithmetic / Arenaire project (LIP, ENS-Lyon)