Avoid pool growth and unaligned mmap attempts when reading FILE buckets

buckets/apr_buckets_file.c

@@ -19,6 +19,10 @@
 #include "apr_file_io.h"
 #include "apr_buckets.h"
 
+#if APR_HAVE_UNISTD_H
+#include <unistd.h> /* for sysconf() */
+#endif
+
 #if APR_HAS_MMAP
 #include "apr_mmap.h"
 
@@ -70,6 +74,39 @@ static int file_make_mmap(apr_bucket *e, apr_size_t filelength,
     file_bucket_destroy(a);
     return 1;
 }
+
+/* Returns the number of bytes to read from the bucket to reach the
+ * next page boundary in the file, or zero if the bucket should not be
+ * split at a page boundary.  A bucket which does not start on a page
+ * boundary, and which cannot be read in a single read regardless, is
+ * read up to the next page boundary so that the remainder of the
+ * bucket can be mapped from a page-aligned offset, provided that
+ * remainder is large enough to be mapped. */
+static apr_size_t file_align_length(apr_bucket_file *a, apr_size_t filelength,
+                                    apr_off_t fileoffset)
+{
+#ifdef _SC_PAGESIZE
+    long psize;
+
+    if (!a->can_mmap || filelength <= a->read_size) {
+        return 0;
+    }
+
+    psize = sysconf(_SC_PAGESIZE);
+    if (psize > 0 && (fileoffset & (apr_off_t)(psize - 1)) != 0) {
+        apr_size_t align = (apr_size_t)(psize
+                                        - (fileoffset & (apr_off_t)(psize - 1)));
+
+        /* Only worthwhile if enough of the bucket remains beyond the
+         * page boundary to be mapped. */
+        if (align < filelength
+            && filelength - align >= APR_MMAP_THRESHOLD) {
+            return align;
+        }
+    }
+#endif
+    return 0;
+}
 #endif
 
 static apr_status_t file_bucket_read(apr_bucket *e, const char **str,
@@ -87,7 +124,10 @@ static apr_status_t file_bucket_read(apr_bucket *e, const char **str,
 #endif
 
 #if APR_HAS_MMAP
-    if (file_make_mmap(e, filelength, fileoffset, a->readpool)) {
+    apr_size_t alignlength = file_align_length(a, filelength, fileoffset);
+
+    if (!alignlength
+        && file_make_mmap(e, filelength, fileoffset, a->readpool)) {
         return apr_bucket_read(e, str, len, block);
     }
 #endif
@@ -110,6 +150,11 @@ static apr_status_t file_bucket_read(apr_bucket *e, const char **str,
 
     *str = NULL;  /* in case we die prematurely */
     *len = (filelength > a->read_size) ? a->read_size : filelength;
+#if APR_HAS_MMAP
+    if (alignlength && alignlength < *len) {
+        *len = alignlength;
+    }
+#endif
     buf = apr_bucket_alloc(*len, e->list);
 
     /* Handle offset ... */

mmap/unix/mmap.c

@@ -92,7 +92,6 @@ APR_DECLARE(apr_status_t) apr_mmap_create(apr_mmap_t **new,
 
     if (file == NULL || file->filedes == -1 || file->buffered)
         return APR_EBADF;
-    (*new) = (apr_mmap_t *)apr_pcalloc(cont, sizeof(apr_mmap_t));
 
     if (flag & APR_MMAP_WRITE) {
         native_flags |= PROT_WRITE;
@@ -108,7 +107,6 @@ APR_DECLARE(apr_status_t) apr_mmap_create(apr_mmap_t **new,
         assert(psize > 0 && (psize & (psize - 1)) == 0);
     }
     poffset = offset & (apr_off_t)(psize - 1);
-    (*new)->poffset = poffset;
 #endif
 
     mm = mmap(NULL, size + poffset,
@@ -121,9 +119,11 @@ APR_DECLARE(apr_status_t) apr_mmap_create(apr_mmap_t **new,
         return errno;
     }
 
-    mm = (char *)mm + poffset;
-
-    (*new)->mm = mm;
+    /* Allocate only on success, so that a caller which repeatedly
+     * fails does not consume memory from the pool. */
+    (*new) = (apr_mmap_t *)apr_pcalloc(cont, sizeof(apr_mmap_t));
+    (*new)->poffset = poffset;
+    (*new)->mm = (char *)mm + poffset;
     (*new)->size = size;
     (*new)->cntxt = cont;
     APR_RING_ELEM_INIT(*new, link);

mmap/win32/mmap.c

@@ -77,6 +77,8 @@ APR_DECLARE(apr_status_t) apr_mmap_create(apr_mmap_t **new, apr_file_t *file,
     apr_size_t psize;
     apr_off_t  poffset;
     DWORD offlo, offhi;
+    HANDLE mhandle;
+    void *mv;
 
     if (size == 0)
         return APR_EINVAL;
@@ -102,7 +104,6 @@ APR_DECLARE(apr_status_t) apr_mmap_create(apr_mmap_t **new, apr_file_t *file,
         memblock = si.dwAllocationGranularity;
     }
 
-    *new = apr_pcalloc(cont, sizeof(apr_mmap_t));
     pstart = (offset / memblock) * memblock;
     poffset = offset - pstart;
     psize = (apr_size_t)poffset + size;
@@ -111,26 +112,29 @@ APR_DECLARE(apr_status_t) apr_mmap_create(apr_mmap_t **new, apr_file_t *file,
      * of the mapped region!
      */
 
-    (*new)->mhandle = CreateFileMappingW(file->filehand, NULL, fmaccess,
-                                         0, 0, NULL);
-    if (!(*new)->mhandle || (*new)->mhandle == INVALID_HANDLE_VALUE)
+    mhandle = CreateFileMappingW(file->filehand, NULL, fmaccess, 0, 0, NULL);
+    if (!mhandle || mhandle == INVALID_HANDLE_VALUE)
     {
         *new = NULL;
         return apr_get_os_error();
     }
 
     offlo = (DWORD)pstart;
     offhi = (DWORD)(pstart >> 32);
-    (*new)->mv = MapViewOfFile((*new)->mhandle, mvaccess, offhi,
-                               offlo, psize);
-    if (!(*new)->mv)
+    mv = MapViewOfFile(mhandle, mvaccess, offhi, offlo, psize);
+    if (!mv)
     {
         apr_status_t rv = apr_get_os_error();
-        CloseHandle((*new)->mhandle);
+        CloseHandle(mhandle);
         *new = NULL;
         return rv;
     }
 
+    /* Allocate only on success, so that a caller which repeatedly
+     * fails does not consume memory from the pool. */
+    *new = apr_pcalloc(cont, sizeof(apr_mmap_t));
+    (*new)->mhandle = mhandle;
+    (*new)->mv = mv;
     (*new)->mm = (char *)(*new)->mv + poffset;
     (*new)->size = size;
     (*new)->cntxt = cont;

test/testbuckets.c

@@ -555,6 +555,197 @@ static void test_truncfile(abts_case *tc, void *data)
     apr_bucket_alloc_destroy(ba);
 }
 
+#if APR_HAS_MMAP
+#include "apr_mmap.h"
+
+#if APR_HAVE_UNISTD_H
+#include <unistd.h> /* for sysconf() */
+#endif
+
+/* Byte expected at offset OFF of a file created by make_pattern_file(). */
+#define PATTERN_BYTE(off) ((char)((off) % 251))
+
+/* Make a test file named FNAME of length LEN, opened with FLAGS. */
+static apr_file_t *make_pattern_file(abts_case *tc, const char *fname,
+                                     apr_off_t len, apr_int32_t flags)
+{
+    apr_file_t *f;
+    char buf[8192];
+    apr_off_t off = 0;
+    apr_status_t rv;
+
+    rv = apr_file_open(&f, fname,
+                       APR_FOPEN_WRITE|APR_FOPEN_TRUNCATE|APR_FOPEN_CREATE,
+                       APR_FPROT_OS_DEFAULT, p);
+    APR_ASSERT_SUCCESS(tc, "create test file", rv);
+    if (rv != APR_SUCCESS) {
+        return NULL;
+    }
+
+    while (off < len) {
+        apr_size_t n, chunk = sizeof buf;
+
+        if ((apr_off_t)chunk > len - off) {
+            chunk = (apr_size_t)(len - off);
+        }
+        for (n = 0; n < chunk; n++) {
+            buf[n] = PATTERN_BYTE(off + n);
+        }
+        APR_ASSERT_SUCCESS(tc, "write test file contents",
+                           apr_file_write_full(f, buf, chunk, NULL));
+        off += chunk;
+    }
+    apr_file_close(f);
+
+    rv = apr_file_open(&f, fname, flags, APR_FPROT_OS_DEFAULT, p);
+    APR_ASSERT_SUCCESS(tc, "reopen test file", rv);
+
+    return rv == APR_SUCCESS ? f : NULL;
+}
+
+/* Returns non-zero if the LEN bytes at BUF match those expected at
+ * offset OFF of a file created by make_pattern_file(). */
+static int pattern_match(const char *buf, apr_size_t len, apr_off_t off)
+{
+    apr_size_t n;
+
+    for (n = 0; n < len; n++) {
+        if (buf[n] != PATTERN_BYTE(off + n)) {
+            return 0;
+        }
+    }
+    return 1;
+}
+
+/* A FILE bucket which starts at an offset which is not a multiple of
+ * the page size, and is larger than APR_MMAP_LIMIT, is read up to the
+ * next page boundary and then via a sequence of MMAP buckets. */
+static void test_mmap_unaligned(abts_case *tc, void *data)
+{
+    static const char fname[] = "mmapfile.bin";
+    apr_bucket_alloc_t *ba = apr_bucket_alloc_create(p);
+    apr_bucket_brigade *bb = apr_brigade_create(p, ba);
+    const apr_off_t start = 4321;
+    const apr_size_t tail = 100000;
+    const apr_size_t flen = APR_MMAP_LIMIT + tail;
+    apr_size_t expect[4] = { 0 }; /* expected bucket lengths */
+    int first_mmap = 0; /* index of first MMAP bucket */
+    apr_off_t off = start;
+    apr_file_t *f;
+    apr_bucket *e;
+    int count = 0;
+
+#ifdef _SC_PAGESIZE
+    {
+        apr_size_t psize = (apr_size_t)sysconf(_SC_PAGESIZE);
+
+        /* Assumes the page size is small enough that the boundary is
+         * reached in a single read. */
+        if (psize > APR_BUCKET_BUFF_SIZE) {
+            ABTS_NOT_IMPL(tc, "Skipped: page size too large");
+            return;
+        }
+        expect[count++] = psize - (apr_size_t)start % psize;
+        first_mmap = 1;
+    }
+#endif
+    expect[count++] = APR_MMAP_LIMIT;
+    expect[count] = flen - APR_MMAP_LIMIT - (first_mmap ? expect[0] : 0);
+
+    f = make_pattern_file(tc, fname, start + flen, APR_FOPEN_READ);
+    if (!f) {
+        return;
+    }
+
+    apr_brigade_insert_file(bb, f, start, flen, p);
+
+    count = 0;
+    while ((e = APR_BRIGADE_FIRST(bb)) != APR_BRIGADE_SENTINEL(bb)
+           && count < 3) {
+        const char *buf;
+        apr_size_t len;
+
+        ABTS_ASSERT(tc, "bucket is a FILE bucket before read",
+                    APR_BUCKET_IS_FILE(e));
+        APR_ASSERT_SUCCESS(tc, "read file bucket",
+                           apr_bucket_read(e, &buf, &len, APR_BLOCK_READ));
+        if (count < first_mmap) {
+            ABTS_ASSERT(tc, "FILE bucket read up to page boundary",
+                        !APR_BUCKET_IS_MMAP(e));
+        }
+        else {
+            ABTS_ASSERT(tc, "FILE bucket morphed to MMAP bucket",
+                        APR_BUCKET_IS_MMAP(e));
+        }
+        ABTS_SIZE_EQUAL(tc, expect[count], len);
+        ABTS_ASSERT(tc, "bucket contents match",
+                    pattern_match(buf, len, off));
+
+        off += len;
+        count++;
+        apr_bucket_delete(e);
+    }
+
+    ABTS_ASSERT(tc, "read whole file", off == start + (apr_off_t)flen);
+    ABTS_ASSERT(tc, "brigade is empty", APR_BRIGADE_EMPTY(bb));
+
+    apr_file_close(f);
+    apr_brigade_destroy(bb);
+    apr_bucket_alloc_destroy(ba);
+    apr_file_remove(fname, p);
+}
+
+/* If the file cannot be mmap()ed, the FILE bucket must fall back to
+ * reading the file. */
+static void test_mmap_fail(abts_case *tc, void *data)
+{
+    static const char fname[] = "mmapfile.bin";
+    apr_bucket_alloc_t *ba = apr_bucket_alloc_create(p);
+    apr_bucket_brigade *bb = apr_brigade_create(p, ba);
+    const apr_size_t flen = 3 * APR_BUCKET_BUFF_SIZE;
+    apr_off_t off = 0;
+    apr_file_t *f;
+    apr_bucket *e;
+    const char *buf;
+    apr_size_t len;
+
+    /* apr_mmap_create() fails for a buffered file. */
+    f = make_pattern_file(tc, fname, flen,
+                          APR_FOPEN_READ | APR_FOPEN_BUFFERED);
+    if (!f) {
+        return;
+    }
+
+    e = apr_brigade_insert_file(bb, f, 0, flen, p);
+
+    APR_ASSERT_SUCCESS(tc, "read file bucket",
+                       apr_bucket_read(e, &buf, &len, APR_BLOCK_READ));
+    ABTS_ASSERT(tc, "FILE bucket did not morph to MMAP bucket",
+                !APR_BUCKET_IS_MMAP(e));
+    ABTS_ASSERT(tc, "partial read of file", len > 0 && len < flen);
+
+    ABTS_ASSERT(tc, "remainder is a FILE bucket",
+                APR_BUCKET_NEXT(e) != APR_BRIGADE_SENTINEL(bb)
+                && APR_BUCKET_IS_FILE(APR_BUCKET_NEXT(e)));
+
+    /* The whole file is still readable. */
+    while ((e = APR_BRIGADE_FIRST(bb)) != APR_BRIGADE_SENTINEL(bb)) {
+        APR_ASSERT_SUCCESS(tc, "read bucket",
+                           apr_bucket_read(e, &buf, &len, APR_BLOCK_READ));
+        ABTS_ASSERT(tc, "bucket contents match",
+                    pattern_match(buf, len, off));
+        off += len;
+        apr_bucket_delete(e);
+    }
+    ABTS_ASSERT(tc, "read whole file", off == (apr_off_t)flen);
+
+    apr_file_close(f);
+    apr_brigade_destroy(bb);
+    apr_bucket_alloc_destroy(ba);
+    apr_file_remove(fname, p);
+}
+#endif /* APR_HAS_MMAP */
+
 static const char hello[] = "hello, world";
 
 static void test_partition(abts_case *tc, void *data)
@@ -674,6 +865,10 @@ abts_suite *testbuckets(abts_suite *suite)
     abts_run_test(suite, test_insertfile, NULL);
     abts_run_test(suite, test_manyfile, NULL);
     abts_run_test(suite, test_truncfile, NULL);
+#if APR_HAS_MMAP
+    abts_run_test(suite, test_mmap_unaligned, NULL);
+    abts_run_test(suite, test_mmap_fail, NULL);
+#endif
     abts_run_test(suite, test_partition, NULL);
     abts_run_test(suite, test_write_split, NULL);
     abts_run_test(suite, test_write_putstrs, NULL);