[PATCH] Fix for issue #1789

Hello,

Windows has a bug in its WriteFile API. When the file handle is a console
output handle and the data chunk is greater than some value around 60 KB,
instead of doing either a complete write (which would be ideal) or a
partial write (which is permitted by the API contract), it returns an
incorrect error causing Subversion to display an error message such as:

svn: Can't write to stream: Not enough storage is available to process this
command.

Clearly, as the actual bug is in Windows, we can't fix the bug itself, so
we need a work-around, and common sense dictates that that work-around be
as close as possible to the API. So, the patch is for APR.

The obvious work-around is to take large writes and divide them up into
smaller writes. APR already uses separate files for file I/O on different
platforms, which means the work-around will be automatically segregated to
Win32 only. Even with the work-around in place, writing a large amount of
data in 48 KB blocks is not going to be significantly slower compared to
the ideal single API call. So, rather than add complexity and try to figure
out whether the file handle is a console file handle, I simply apply the
work-around to *all* file writes done through apr_file_write on Win32.

I've attached a patch which achieves this, however after an hour and a half
of trying to work with the Subversion build system for Windows (which I am
not familiar with), I was not able to get a working build. So, I present
this patch as "carefully reviewed & probably good but untested". I was
careful to preserve the semantics of the border case where *nbytes is
initially 0; as before, the function still calls WriteFile() with a count
of 0 once.

I would like to ask someone who does have a working Win32 build environment
using APR head to try applying my patch and see if it fixes the problem.

There is a test case for the bug at the following URL:

http://israel.logiclrd.cx/svn_bug.zip

Simply extract it to its own subdirectory and then issue "svn diff" within
that directory on a Windows platform. It should fail prior to the patch,
and, in theory, succeed with the patch applied.

If anyone would like to give me some hints on building under Windows, that
would also be great. I used "gen-make.py -t vcproj" as per the
instructions, but ended up with projects that do not reference their
dependencies and DLLs that do not export any functions at all.

Without further ado, here is the patch (inline & attached as I expect the
inline copy to be mangled by my e-mail client):

[[[
Fix issue #1789 by working around the underlying Windows bug.

* file_io/win32/readwrite.c: Made apr_file_write divide large buffers into
  smaller chunks.
]]]

Index: file_io/win32/readwrite.c
===================================================================
--- file_io/win32/readwrite.c (revision 509010)
+++ file_io/win32/readwrite.c (working copy)
@@ -24,6 +24,16 @@
 #include "apr_arch_atime.h"
 #include "apr_arch_misc.h"
 
+/* This value is used to divide write operations into smaller
+ * chunks due to a bug in Windows that occurs when WriteFile
+ * is used on chunks larger than 64KB. Instead of doing a
+ * partial write (or internally looping in order to do a full
+ * write), it returns an error:
+ *
+ * "Not enough storage is available to process this command."
+ */
+#define MAXIMUM_SYNCHRONOUS_WRITE_SIZE (48 * 1024)
+
 /*
  * read_with_timeout()
  * Uses async i/o to emulate unix non-blocking i/o with timeouts.
@@ -309,9 +319,48 @@
             if (thefile->pOverlapped) {
                 thefile->pOverlapped->Offset = (DWORD)thefile->filePtr;
                 thefile->pOverlapped->OffsetHigh =
(DWORD)(thefile->filePtr >> 32);
+ rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes,
+ &bwrote, thefile->pOverlapped);
             }
- rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, &bwrote,
- thefile->pOverlapped);
+ else {
+ /* The write may need to be divided up into chunks due to a
+ * bug in WriteFile when it is given a console file handle.
+ */
+ bwrote = 0;
+ do {
+ /* Determine the next chunk's size and write it. */
+ DWORD chunk_size = MAXIMUM_SYNCHRONOUS_WRITE_SIZE;
+
+ if (chunk_size > *nbytes) {
+ chunk_size = *nbytes;
+ }
+
+ rv = WriteFile(thefile->filehand, buf,
+ chunk_size, &chunk_size, NULL);
+
+ /* If an error occurs but some bytes were written, report
+ * success for those bytes only. A subsequent call will
+ * return the same error without any bytes having been
+ * written.
+ */
+ if (!rv) {
+ if (bwrote > 0) {
+ /* By making rv non-zero, the return-value test
+ * below will return APR_SUCCESS.
+ */
+ rv = TRUE;
+ }
+ break;
+ }
+
+ /* Tally the bytes that were written and advance the
+ * buffer pointer. */
+ bwrote += chunk_size;
+
+ buf = chunk_size + (char *)buf;
+ *nbytes -= chunk_size;
+ } while (*nbytes > 0);
+ }
             if (thefile->append) {
                 apr_file_unlock(thefile);
                 apr_thread_mutex_unlock(thefile->mutex);

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org