https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=344860a1045cbb8ef1f3caf265a9d706cdda01e0
commit 344860a1045cbb8ef1f3caf265a9d706cdda01e0 Author: Corinna Vinschen <[email protected]> Date: Sat Aug 15 12:30:09 2015 +0200 Cygwin: Try to fix potential data corruption in pipe write * fhandler.cc (fhandler_base_overlapped::raw_write): When performing nonblocking I/O, copy user space data into own buffer. Add longish comment to explain why. * fhandler.h (fhandler_base_overlapped::atomic_write_buf): New member. (fhandler_base_overlapped::fhandler_base_overlapped): Initialize atomic_write_buf. (fhandler_base_overlapped::fhandler_base_overlapped): New destructor, free'ing atomic_write_buf. (fhandler_base_overlapped::copyto): Set atomic_write_buf to NULL in copied fhandler. Signed-off-by: Corinna Vinschen <[email protected]> Diff: --- winsup/cygwin/ChangeLog | 13 +++++++++++++ winsup/cygwin/fhandler.cc | 40 ++++++++++++++++++++++++++++++++++++++++ winsup/cygwin/fhandler.h | 9 ++++++++- winsup/cygwin/release/2.2.1 | 5 +++++ 4 files changed, 66 insertions(+), 1 deletion(-) diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index 274ec53..6b82e32 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,16 @@ +2015-08-15 Corinna Vinschen <[email protected]> + + * fhandler.cc (fhandler_base_overlapped::raw_write): When performing + nonblocking I/O, copy user space data into own buffer. Add longish + comment to explain why. + * fhandler.h (fhandler_base_overlapped::atomic_write_buf): New member. + (fhandler_base_overlapped::fhandler_base_overlapped): Initialize + atomic_write_buf. + (fhandler_base_overlapped::fhandler_base_overlapped): New destructor, + free'ing atomic_write_buf. + (fhandler_base_overlapped::copyto): Set atomic_write_buf to NULL in + copied fhandler. + 2015-08-14 Corinna Vinschen <[email protected]> * security.cc (convert_samba_sd): Fix copy/paste error in previous diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc index 6f024da..4343cdf 100644 --- a/winsup/cygwin/fhandler.cc +++ b/winsup/cygwin/fhandler.cc @@ -2093,6 +2093,46 @@ fhandler_base_overlapped::raw_write (const void *ptr, size_t len) else chunk = max_atomic_write; + /* MSDN "WriteFile" contains the following note: "Accessing the output + buffer while a write operation is using the buffer may lead to + corruption of the data written from that buffer. [...] This can + be particularly problematic when using an asynchronous file handle. + (https://msdn.microsoft.com/en-us/library/windows/desktop/aa365747) + + MSDN "Synchronous and Asynchronous I/O" contains the following note: + "Do not deallocate or modify [...] the data buffer until all + asynchronous I/O operations to the file object have been completed." + (https://msdn.microsoft.com/en-us/library/windows/desktop/aa365683) + + This problem is a non-issue for blocking I/O, but it can lead to + problems when using nonblocking I/O. Consider: + - The application uses a static buffer in repeated calls to + non-blocking write. + - The previous write returned with success, but the overlapped I/O + operation is ongoing. + - The application copies the next set of data to the static buffer, + thus overwriting data still accessed by the previous write call. + --> potential data corruption. + + What we do here is to allocate a per-fhandler buffer big enough + to perform the maximum atomic operation from, copy the user space + data over to this buffer and then call NtWriteFile on this buffer. + This decouples the write operation from the user buffer and the + user buffer can be reused without data corruption issues. + + Since no further write can occur while we're still having ongoing + I/O, this should be reasanably safe. + + Note: We only have proof that this problem actually occurs on Wine + yet. However, the MSDN language indicates that this may be a real + problem on real Windows as well. */ + if (is_nonblocking ()) + { + if (!atomic_write_buf) + atomic_write_buf = cmalloc_abort (HEAP_BUF, max_atomic_write); + ptr = memcpy (atomic_write_buf, ptr, chunk); + } + nbytes = 0; DWORD nbytes_now = 0; /* Write to fd in smaller chunks, accumulating a total. diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index e15f946..6e964aa 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -661,6 +661,7 @@ protected: OVERLAPPED io_status; OVERLAPPED *overlapped; size_t max_atomic_write; + void *atomic_write_buf; public: wait_return __reg3 wait_overlapped (bool, bool, DWORD *, bool, DWORD = 0); int __reg1 setup_overlapped (); @@ -670,7 +671,7 @@ public: OVERLAPPED *&get_overlapped () {return overlapped;} OVERLAPPED *get_overlapped_buffer () {return &io_status;} void set_overlapped (OVERLAPPED *ov) {overlapped = ov;} - fhandler_base_overlapped (): io_pending (false), overlapped (NULL), max_atomic_write (0) + fhandler_base_overlapped (): io_pending (false), overlapped (NULL), max_atomic_write (0), atomic_write_buf (NULL) { memset (&io_status, 0, sizeof io_status); } @@ -686,11 +687,17 @@ public: static void __reg1 flush_all_async_io ();; fhandler_base_overlapped (void *) {} + ~fhandler_base_overlapped () + { + if (atomic_write_buf) + cfree (atomic_write_buf); + } virtual void copyto (fhandler_base *x) { x->pc.free_strings (); *reinterpret_cast<fhandler_base_overlapped *> (x) = *this; + reinterpret_cast<fhandler_base_overlapped *> (x)->atomic_write_buf = NULL; x->reset (this); } diff --git a/winsup/cygwin/release/2.2.1 b/winsup/cygwin/release/2.2.1 index b31f182..4aa797a 100644 --- a/winsup/cygwin/release/2.2.1 +++ b/winsup/cygwin/release/2.2.1 @@ -19,3 +19,8 @@ Bug Fixes - Don't try to perform RFC2307 owner/group mapping on Samba/NFS if account info is only fetched from local passwd/group files. Addresses: https://cygwin.com/ml/cygwin/2015-07/msg00270.html + +- Precautionally fix a potential data corruption problem in pipe I/O, only + actually observered in Wine yet. However, MSDN language indicates this + might be a problem on real Windows as well. + Addresses: Freenode IRC #cygwin discussion, ML entry follows.
