[prev in list] [next in list] [prev in thread] [next in thread] 

List:       apr-dev
Subject:    [PATCH] Win32 fix for corrupted byterange reply
From:       Bill Stoddard <bill () wstoddard ! com>
Date:       2004-05-28 13:16:24
Message-ID: 40B73BA8.9040800 () wstoddard ! com
[Download RAW message or body]

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;


[prev in list] [next in list] [prev in thread] [next in thread]