Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Message-ID: <40B73BA8.9040800@wstoddard.com>
Date: Fri, 28 May 2004 09:16:24 -0400
From: Bill Stoddard <bill@wstoddard.com>
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
 rv:1.6) Gecko/20040113
MIME-Version: 1.0
To: dev@apr.apache.org
Subject: [PATCH] Win32 fix for corrupted byterange reply
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

Failure scenario:
default_handler opens a pdf file to be sent via apr_sendfile (in other words, the file is opened for 
overlapped i/o aka APR_XTHREAD).

An output filter is loaded (SSL in this case) that prevents apr_sendfile() from being used so we need to call 
apr_file_read() to read the file contents into a bucket. When a windows file is opened for overlapped i/o, the 
user application (APR in this case) must explicitly maintain the file pointer. The bug is that there is an 
edge case where APR does not properly maintain the file pointer. If apr_file_seek() is called on a file opened 
for overlapped i/o -before- apr_file_read(), the filepointer (maintained in apr_file_t) will not be properly 
updated to the value passed in on apr_file_seek(). The subsequent apr_file_read() will read beginning at the 
wrong offset and the pdf file will not be properly rendered.

There are two pieces to this fix. The key piece is here:
Index: seek.c
===================================================================
RCS file: /cvs/phoenix/2.0.47/srclib/apr/file_io/win32/seek.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 seek.c
--- seek.c    29 Jul 2003 20:19:26 -0000    1.1.1.1
+++ seek.c    27 May 2004 18:07:18 -0000
@@ -122,7 +122,10 @@
          *offset = thefile->filePtr - thefile->dataRead + thefile->bufpos;
          return rc;
      }
-    else if (thefile->pOverlapped) {
+    /* A file opened with APR_XTHREAD has been opened for overlapped i/o.
+     * APR must explicitly track the file pointer in this case.
+     */
+    else if (thefile->flags & APR_XTHREAD) {
          switch(where) {
              case APR_SET:
                  thefile->filePtr = *offset;

This change will cause the filepointer (thfile->filePtr) to be properly set on the first apr_file_seek(). This 
  single change will enable the file to be served up correctly.

A secondary issue I'd like to address is when the overlapped structure is created. Prior to this patch, the 
overlapped structure (thefile->pOverlapped) was created in either apr_file_read() or apr_file_write(). The 
following two changes move the creation of pOverlapped to apr_file_open().  Anyone see any problems with this?

Index: open.c
===================================================================
RCS file: /cvs/phoenix/2.0.47/srclib/apr/file_io/win32/open.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 open.c
--- open.c    29 Jul 2003 20:19:26 -0000    1.1.1.1
+++ open.c    27 May 2004 18:07:18 -0000
@@ -437,6 +437,23 @@
          apr_pool_cleanup_register((*new)->pool, (void *)(*new), file_cleanup,
                                    apr_pool_cleanup_null);
      }
+
+    /* Create the overlapped structure if the file is opened for overlapped
+     * i/o. Files opened for APR_XTHREAD support should always be opened for
+     * overlapped i/o and all files opened for overlapped i/o should be marked
+     * as APR_XTHREAD files. Note: Threads should not share an apr_file_t or
+     * its hEvent.
+     */
+    if ((*new)->flags & APR_XTHREAD) {
+        (*new)->pOverlapped = (OVERLAPPED*) apr_pcalloc((*new)->pool,
+                                                        sizeof(OVERLAPPED));
+        (*new)->pOverlapped->hEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
+        if (!(*new)->pOverlapped->hEvent) {
+            rv = apr_get_os_error();
+            return rv;
+        }
+    }
+
      return APR_SUCCESS;
  }

Index: readwrite.c
===================================================================
RCS file: /cvs/phoenix/2.0.47/srclib/apr/file_io/win32/readwrite.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 readwrite.c
--- readwrite.c    29 Jul 2003 20:19:26 -0000    1.1.1.1
+++ readwrite.c    27 May 2004 18:07:18 -0000
@@ -168,20 +168,6 @@
          return APR_SUCCESS;
      }

-    /* If the file is open for xthread support, allocate and
-     * initialize the overlapped and io completion event (hEvent).
-     * Threads should NOT share an apr_file_t or its hEvent.
-     */
-    if ((thefile->flags & APR_XTHREAD) && !thefile->pOverlapped ) {
-        thefile->pOverlapped = (OVERLAPPED*) apr_pcalloc(thefile->pool,
-                                                         sizeof(OVERLAPPED));
-        thefile->pOverlapped->hEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
-        if (!thefile->pOverlapped->hEvent) {
-            rv = apr_get_os_error();
-            return rv;
-        }
-    }
-
      /* Handle the ungetchar if there is one */
      if (thefile->ungetchar != -1) {
          bytes_read = 1;
@@ -252,20 +238,6 @@
  {
      apr_status_t rv;
      DWORD bwrote;
-
-    /* If the file is open for xthread support, allocate and
-     * initialize the overlapped and io completion event (hEvent).
-     * Threads should NOT share an apr_file_t or its hEvent.
-     */
-    if ((thefile->flags & APR_XTHREAD) && !thefile->pOverlapped ) {
-        thefile->pOverlapped = (OVERLAPPED*) apr_pcalloc(thefile->pool,
-                                                         sizeof(OVERLAPPED));
-        thefile->pOverlapped->hEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
-        if (!thefile->pOverlapped->hEvent) {
-            rv = apr_get_os_error();
-            return rv;
-        }
-    }

      if (thefile->buffered) {
          char *pos = (char *)buf;