[newlib-cygwin] Cygwin: Try to fix potential data corruption in pipe write
Corinna Vinschen
corinna@sourceware.org
Sat Aug 15 10:30:00 GMT 2015
https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=344860a1045cbb8ef1f3caf265a9d706cdda01e0
commit 344860a1045cbb8ef1f3caf265a9d706cdda01e0
Author: Corinna Vinschen <corinna@vinschen.de>
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 <corinna@vinschen.de>
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 <corinna@vinschen.de>
+
+ * 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 <corinna@vinschen.de>
* 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.
More information about the Cygwin-cvs
mailing list