[PATCH] Fix a deadlock when appending to locked files on Windows (PR50058)

[Evgeny Kotkov <ev...@visualsvn.com>]

Hi everyone,

Currently, appending data to a file opened with APR_FOPEN_APPEND
that has been locked by apr_file_lock() will cause a deadlock on Windows.

[See PR50058, https://bz.apache.org/bugzilla/show_bug.cgi?id=50058]

This issue happens because atomic O_APPEND-style appends on Windows
are implemented using file locks.  An append happens while holding the
file lock acquired with LockFile(), which is required to avoid a race
condition between seeking to the end of file and writing data.  The
race is possible when multiple threads or processes are appending
data to the same file.

This approach causes a deadlock if the file has been previously locked
with apr_file_lock().  (Note that it's perfectly legit to lock the file or
its portion and perform the append after that.)

Apart from this, using file locks for file appends impacts their speed and
robustness.  There is an overhead associated with locking the file, especially
if the file is not local.  The robustness is affected, because other writes
to the same file may fail due to it being locked.  Also, if a process is
terminated in the middle of the append operation, it might take some time
for the OS to release the file lock.  During this time, the file would be
inaccessible to other processes.  This may affect applications such as
httpd (with a multi-process MPM) that use APR_FOPEN_APPEND files for
logging.

This patch series fixes the issue by switching to the documented way to
atomically append data with a single WriteFile() call.  It requires passing
special OVERLAPPED.Offset and OffsetHigh values (0xFFFFFFFF).  On the
ZwWriteFile() layer, this maps to a FILE_WRITE_TO_END_OF_FILE constant
that instructs the OS (the corresponding file system driver) to write data
to the file's end.

Note that this approach is only used for files opened for synchronous I/O
because in this case the I/O Manager maintains the current file position.
Otherwise, the file offset returned or changed by the SetFilePointer() API is
not guaranteed to be valid and that could, for instance, break apr_file_seek()
calls after appending data.  Sadly, if a file is opened for asynchronous I/O,
this call to WriteFile() doesn't update the OVERLAPPED.Offset member to
reflect the actual offset used when appending the data (which we could
then use to make seeking and other operations involving filePtr work).
Therefore, when appending to files opened for asynchronous I/O, we still
use the old LockFile + SetFilePointer + WriteFile approach.

Additional details on this can be found in:

  https://msdn.microsoft.com/en-us/library/windows/desktop/aa365747
  https://msdn.microsoft.com/en-us/library/windows/hardware/ff567121


The implementation is split into four dependent patches.  The first three
patches are minor refactorings to the apr_file_write() function and lay the
necessary groundwork for the core change.  The fourth patch is the core
change that fixes the issue in the described way and also includes the
tests.

The log messages are included in the beginning of each patch file.


Thanks,
Evgeny Kotkov

Re: [PATCH] Fix a deadlock when appending to locked files on Windows (PR50058)

[Ivan Zhakov <iv...@visualsvn.com>]

On 28 August 2017 at 13:44, Evgeny Kotkov <ev...@visualsvn.com> wrote:
> Hi everyone,
>
> Currently, appending data to a file opened with APR_FOPEN_APPEND
> that has been locked by apr_file_lock() will cause a deadlock on Windows.
>
> [See PR50058, https://bz.apache.org/bugzilla/show_bug.cgi?id=50058]
>
> This issue happens because atomic O_APPEND-style appends on Windows
> are implemented using file locks.  An append happens while holding the
> file lock acquired with LockFile(), which is required to avoid a race
> condition between seeking to the end of file and writing data.  The
> race is possible when multiple threads or processes are appending
> data to the same file.
>
> This approach causes a deadlock if the file has been previously locked
> with apr_file_lock().  (Note that it's perfectly legit to lock the file or
> its portion and perform the append after that.)
>
> Apart from this, using file locks for file appends impacts their speed and
> robustness.  There is an overhead associated with locking the file, especially
> if the file is not local.  The robustness is affected, because other writes
> to the same file may fail due to it being locked.  Also, if a process is
> terminated in the middle of the append operation, it might take some time
> for the OS to release the file lock.  During this time, the file would be
> inaccessible to other processes.  This may affect applications such as
> httpd (with a multi-process MPM) that use APR_FOPEN_APPEND files for
> logging.
>
> This patch series fixes the issue by switching to the documented way to
> atomically append data with a single WriteFile() call.  It requires passing
> special OVERLAPPED.Offset and OffsetHigh values (0xFFFFFFFF).  On the
> ZwWriteFile() layer, this maps to a FILE_WRITE_TO_END_OF_FILE constant
> that instructs the OS (the corresponding file system driver) to write data
> to the file's end.
>
> Note that this approach is only used for files opened for synchronous I/O
> because in this case the I/O Manager maintains the current file position.
> Otherwise, the file offset returned or changed by the SetFilePointer() API is
> not guaranteed to be valid and that could, for instance, break apr_file_seek()
> calls after appending data.  Sadly, if a file is opened for asynchronous I/O,
> this call to WriteFile() doesn't update the OVERLAPPED.Offset member to
> reflect the actual offset used when appending the data (which we could
> then use to make seeking and other operations involving filePtr work).
> Therefore, when appending to files opened for asynchronous I/O, we still
> use the old LockFile + SetFilePointer + WriteFile approach.
>
> Additional details on this can be found in:
>
>   https://msdn.microsoft.com/en-us/library/windows/desktop/aa365747
>   https://msdn.microsoft.com/en-us/library/windows/hardware/ff567121
>
>
> The implementation is split into four dependent patches.  The first three
> patches are minor refactorings to the apr_file_write() function and lay the
> necessary groundwork for the core change.  The fourth patch is the core
> change that fixes the issue in the described way and also includes the
> tests.
>
> The log messages are included in the beginning of each patch file.
>
> 1-file-write-simplify-local-vars-v1.patch.txt
Committed in r1806592.

> 2-file-write-simplify-fileptr-inc-v1.patch.txt
Committed in r1806603.


> 3-file-write-invert-if-condition-v1.patch.txt
Committed in r1806604.

> 4-fix-locked-append-deadlock-v1.patch.txt
Committed in r1806608.

Thanks!

-- 
Ivan Zhakov