You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by yl...@apache.org on 2021/03/02 00:37:18 UTC
svn commit: r1887060 - in /apr/apr/trunk: CHANGES include/apr_mmap.h
mmap/unix/mmap.c test/data/mmap_large_datafile.txt test/testmmap.c
Author: ylavic
Date: Tue Mar 2 00:37:18 2021
New Revision: 1887060
URL: http://svn.apache.org/viewvc?rev=1887060&view=rev
Log:
Align apr_mmap()ing offset to a page boundary. PR 65158.
This is requirement for unix systems, and otherwise can cause failures or
undefined behaviour with some filesystems.
Add apr_mmap_t->poffset on unixes to track the offset to the previous page,
relative to the user requested/actual offset. This allows ->mm and ->size to
still point to the actual data, while munmap() still deletes the full range
from the start of the page.
Tests updated accordingly.
Added:
apr/apr/trunk/test/data/mmap_large_datafile.txt
Modified:
apr/apr/trunk/CHANGES
apr/apr/trunk/include/apr_mmap.h
apr/apr/trunk/mmap/unix/mmap.c
apr/apr/trunk/test/testmmap.c
Modified: apr/apr/trunk/CHANGES
URL: http://svn.apache.org/viewvc/apr/apr/trunk/CHANGES?rev=1887060&r1=1887059&r2=1887060&view=diff
==============================================================================
--- apr/apr/trunk/CHANGES [utf-8] (original)
+++ apr/apr/trunk/CHANGES [utf-8] Tue Mar 2 00:37:18 2021
@@ -1,6 +1,8 @@
-*- coding: utf-8 -*-
Changes for APR 2.0.0
+ *) Align apr_mmap()ing offset to a page boundary. [Yann Ylavic]
+
*) APR's configure script uses AC_TRY_RUN to detect whether the return type
of strerror_r is int. When cross-compiling this defaults to no.
Modified: apr/apr/trunk/include/apr_mmap.h
URL: http://svn.apache.org/viewvc/apr/apr/trunk/include/apr_mmap.h?rev=1887060&r1=1887059&r2=1887060&view=diff
==============================================================================
--- apr/apr/trunk/include/apr_mmap.h (original)
+++ apr/apr/trunk/include/apr_mmap.h Tue Mar 2 00:37:18 2021
@@ -62,11 +62,10 @@ typedef struct apr_mmap_t apr
struct apr_mmap_t {
/** The pool the mmap structure was allocated out of. */
apr_pool_t *cntxt;
-#ifdef BEOS
+#if defined(BEOS)
/** An area ID. Only valid on BeOS */
area_id area;
-#endif
-#ifdef WIN32
+#elif defined(WIN32)
/** The handle of the file mapping */
HANDLE mhandle;
/** The start of the real memory page area (mapped view) */
@@ -75,6 +74,8 @@ struct apr_mmap_t {
apr_off_t pstart;
apr_size_t psize;
apr_off_t poffset;
+#else
+ apr_off_t poffset;
#endif
/** The start of the memory mapped area */
void *mm;
Modified: apr/apr/trunk/mmap/unix/mmap.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/mmap/unix/mmap.c?rev=1887060&r1=1887059&r2=1887060&view=diff
==============================================================================
--- apr/apr/trunk/mmap/unix/mmap.c (original)
+++ apr/apr/trunk/mmap/unix/mmap.c Tue Mar 2 00:37:18 2021
@@ -14,6 +14,8 @@
* limitations under the License.
*/
+#include <assert.h>
+
#include "apr.h"
#include "apr_private.h"
#include "apr_general.h"
@@ -33,6 +35,9 @@
#if APR_HAVE_STDIO_H
#include <stdio.h>
#endif
+#if APR_HAVE_UNISTD_H
+#include <unistd.h> /* for sysconf() */
+#endif
#ifdef HAVE_SYS_STAT_H
#include <sys/stat.h>
#endif
@@ -61,7 +66,7 @@ static apr_status_t mmap_cleanup(void *t
#ifdef BEOS
rv = delete_area(mm->area);
#else
- rv = munmap(mm->mm, mm->size);
+ rv = munmap((char *)mm->mm - mm->poffset, mm->size + mm->poffset);
#endif
mm->mm = (void *)-1;
@@ -81,6 +86,8 @@ APR_DECLARE(apr_status_t) apr_mmap_creat
area_id aid = -1;
uint32 pages = 0;
#else
+ static long psize;
+ apr_off_t poffset = 0;
apr_int32_t native_flags = 0;
#endif
@@ -130,7 +137,18 @@ APR_DECLARE(apr_status_t) apr_mmap_creat
native_flags |= PROT_READ;
}
- mm = mmap(NULL, size, native_flags, MAP_SHARED, file->filedes, offset);
+#if defined(_SC_PAGESIZE)
+ if (psize == 0) {
+ psize = sysconf(_SC_PAGESIZE);
+ /* the page size should be a power of two */
+ assert(psize > 0 && (psize & (psize - 1)) == 0);
+ }
+ poffset = offset & (apr_off_t)(psize - 1);
+#endif
+
+ mm = mmap(NULL, size + poffset,
+ native_flags, MAP_SHARED,
+ file->filedes, offset - poffset);
if (mm == (void *)-1) {
/* we failed to get an mmap'd file... */
@@ -139,7 +157,8 @@ APR_DECLARE(apr_status_t) apr_mmap_creat
}
#endif
- (*new)->mm = mm;
+ (*new)->mm = (char *)mm + poffset;
+ (*new)->poffset = poffset;
(*new)->size = size;
(*new)->cntxt = cont;
APR_RING_ELEM_INIT(*new, link);
Added: apr/apr/trunk/test/data/mmap_large_datafile.txt
URL: http://svn.apache.org/viewvc/apr/apr/trunk/test/data/mmap_large_datafile.txt?rev=1887060&view=auto
==============================================================================
--- apr/apr/trunk/test/data/mmap_large_datafile.txt (added)
+++ apr/apr/trunk/test/data/mmap_large_datafile.txt Tue Mar 2 00:37:18 2021
@@ -0,0 +1,2 @@
[... 4 lines stripped ...]
Modified: apr/apr/trunk/test/testmmap.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/test/testmmap.c?rev=1887060&r1=1887059&r2=1887060&view=diff
==============================================================================
--- apr/apr/trunk/test/testmmap.c (original)
+++ apr/apr/trunk/test/testmmap.c Tue Mar 2 00:37:18 2021
@@ -35,18 +35,30 @@ static void not_implemented(abts_case *t
#else
-static char test_string[256]; /* read from the datafile */
+static apr_pool_t *ptest;
+static char *thisfdata; /* read from the datafile */
static apr_mmap_t *themmap = NULL;
static apr_file_t *thefile = NULL;
static char *file1;
static apr_finfo_t thisfinfo;
static apr_size_t thisfsize;
+static struct {
+ const char *filename;
+ apr_off_t offset;
+} test_set[] = {
+ { "/data/mmap_datafile.txt", 0 },
+ { "/data/mmap_large_datafile.txt", 65536 },
+ { "/data/mmap_large_datafile.txt", 66650 }, /* not page aligned */
+ { NULL, }
+};
+
static void create_filename(abts_case *tc, void *data)
{
+ const char *filename = data;
char *oldfileptr;
- apr_filepath_get(&file1, 0, p);
+ apr_filepath_get(&file1, 0, ptest);
#ifndef NETWARE
#ifdef WIN32
ABTS_TRUE(tc, file1[1] == ':');
@@ -57,7 +69,7 @@ static void create_filename(abts_case *t
ABTS_TRUE(tc, file1[strlen(file1) - 1] != '/');
oldfileptr = file1;
- file1 = apr_pstrcat(p, file1,"/data/mmap_datafile.txt" ,NULL);
+ file1 = apr_pstrcat(ptest, file1, filename, NULL);
ABTS_TRUE(tc, oldfileptr != file1);
}
@@ -67,25 +79,15 @@ static void test_file_close(abts_case *t
rv = apr_file_close(thefile);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ thefile = NULL;
}
-static void read_expected_contents(abts_case *tc, void *data)
-{
- apr_status_t rv;
- apr_size_t nbytes = sizeof(test_string) - 1;
-
- rv = apr_file_read(thefile, test_string, &nbytes);
- ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
- test_string[nbytes] = '\0';
- thisfsize = strlen(test_string);
-}
-
static void test_file_open(abts_case *tc, void *data)
{
apr_status_t rv;
rv = apr_file_open(&thefile, file1, APR_FOPEN_READ,
- APR_FPROT_UREAD | APR_FPROT_GREAD, p);
+ APR_FPROT_UREAD | APR_FPROT_GREAD, ptest);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
ABTS_PTR_NOTNULL(tc, thefile);
}
@@ -96,28 +98,54 @@ static void test_get_filesize(abts_case
rv = apr_file_info_get(&thisfinfo, APR_FINFO_NORM, thefile);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
- ABTS_ASSERT(tc, "File size mismatch", thisfsize == thisfinfo.size);
+
+ thisfsize = thisfinfo.size;
+ thisfdata = apr_palloc(ptest, thisfsize + 1);
+ ABTS_PTR_NOTNULL(tc, thisfdata);
+}
+
+static void read_expected_contents(abts_case *tc, void *data)
+{
+ apr_off_t *offset = data;
+ apr_size_t nbytes = 0;
+ apr_status_t rv;
+
+ rv = apr_file_read_full(thefile, thisfdata, thisfsize, &nbytes);
+ ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ ABTS_ASSERT(tc, "File size mismatch", nbytes == thisfsize);
+ thisfdata[nbytes] = '\0';
+ ABTS_ASSERT(tc, "File content size mismatch",
+ strlen(thisfdata) == thisfsize);
+ ABTS_ASSERT(tc, "File size too small",
+ (apr_size_t)*offset < thisfsize);
+
+ /* From now, pretend that the file data and size don't include the
+ * offset, this avoids adding/substrating it to thisfdata/thisfsize
+ * all over the place in the next tests.
+ */
+ thisfdata += *offset;
+ thisfsize -= *offset;
}
static void test_mmap_create(abts_case *tc, void *data)
{
+ apr_off_t *offset = data;
apr_status_t rv;
- rv = apr_mmap_create(&themmap, thefile, 0, (apr_size_t) thisfinfo.size,
- APR_MMAP_READ, p);
+ rv = apr_mmap_create(&themmap, thefile, *offset, thisfsize,
+ APR_MMAP_READ, ptest);
ABTS_PTR_NOTNULL(tc, themmap);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
}
static void test_mmap_contents(abts_case *tc, void *data)
{
-
ABTS_PTR_NOTNULL(tc, themmap);
ABTS_PTR_NOTNULL(tc, themmap->mm);
ABTS_SIZE_EQUAL(tc, thisfsize, themmap->size);
/* Must use nEquals since the string is not guaranteed to be NULL terminated */
- ABTS_STR_NEQUAL(tc, themmap->mm, test_string, thisfsize);
+ ABTS_STR_NEQUAL(tc, themmap->mm, thisfdata, thisfsize);
}
static void test_mmap_delete(abts_case *tc, void *data)
@@ -127,6 +155,7 @@ static void test_mmap_delete(abts_case *
ABTS_PTR_NOTNULL(tc, themmap);
rv = apr_mmap_delete(themmap);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ themmap = NULL;
}
static void test_mmap_offset(abts_case *tc, void *data)
@@ -139,8 +168,9 @@ static void test_mmap_offset(abts_case *
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
/* Must use nEquals since the string is not guaranteed to be NULL terminated */
- ABTS_STR_NEQUAL(tc, addr, test_string + 5, thisfsize-5);
+ ABTS_STR_NEQUAL(tc, addr, thisfdata + 5, thisfsize - 5);
}
+
#endif
abts_suite *testmmap(abts_suite *suite)
@@ -148,15 +178,21 @@ abts_suite *testmmap(abts_suite *suite)
suite = ADD_SUITE(suite)
#if APR_HAS_MMAP
- abts_run_test(suite, create_filename, NULL);
- abts_run_test(suite, test_file_open, NULL);
- abts_run_test(suite, read_expected_contents, NULL);
- abts_run_test(suite, test_get_filesize, NULL);
- abts_run_test(suite, test_mmap_create, NULL);
- abts_run_test(suite, test_mmap_contents, NULL);
- abts_run_test(suite, test_mmap_offset, NULL);
- abts_run_test(suite, test_mmap_delete, NULL);
- abts_run_test(suite, test_file_close, NULL);
+ apr_size_t i;
+ apr_pool_create(&ptest, p);
+ for (i = 0; test_set[i].filename; ++i) {
+ abts_run_test(suite, create_filename, (void *)test_set[i].filename);
+ abts_run_test(suite, test_file_open, NULL);
+ abts_run_test(suite, test_get_filesize, NULL);
+ abts_run_test(suite, read_expected_contents, &test_set[i].offset);
+ abts_run_test(suite, test_mmap_create, &test_set[i].offset);
+ abts_run_test(suite, test_mmap_contents, &test_set[i].offset);
+ abts_run_test(suite, test_mmap_offset, &test_set[i].offset);
+ abts_run_test(suite, test_mmap_delete, NULL);
+ abts_run_test(suite, test_file_close, NULL);
+ apr_pool_clear(ptest);
+ }
+ apr_pool_destroy(ptest);
#else
abts_run_test(suite, not_implemented, NULL);
#endif
Re: svn commit: r1887060 - in /apr/apr/trunk: CHANGES
include/apr_mmap.h mmap/unix/mmap.c test/data/mmap_large_datafile.txt test/testmmap.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Mar 2, 2021 at 8:35 AM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 3/2/21 1:54 AM, Yann Ylavic wrote:
> >
> > By adding this "poffset" member to apr_mmap_t (on unixes), its layout changes.
> > Can I backport this to 1.7 or should I find another way to store
> > "poffset" there (like some pool/cntxt userdata)?
>
> I don't think that this can be backported this way as it changes a non opaque struct on the public API.
> So you would need to look for a different area to store this data.
That's what I feared :)
Two proposals then (patches attached):
1. pool userdata
Should work for any apr_mmap_t, even if it's not created by apr_mmap_create().
It's slower on delete/cleanup though, and "leaks" a userdata key on
the pool for each mmap created.
2. layout struct { apr_mmap_t mm; apr_off_t poffset; } internal to
mmap/unix/mmap.c
This one is as effective as trunk's version, but requires that the
apr_mmap_t be apr_mmap_create()d (not allocated by the user or on the
stack).
It's not a problem with apr_mmap_delete() because it's a noop for an
handcrafted apr_mmap_t (no mmap_cleanup registered), however
apr_mmap_dup() may read above the limits..
How much do we consider valid an apr_mmap_t not apr_mmap_create()d?
Regards;
Yann.
Re: svn commit: r1887060 - in /apr/apr/trunk: CHANGES
include/apr_mmap.h mmap/unix/mmap.c test/data/mmap_large_datafile.txt
test/testmmap.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 3/2/21 1:54 AM, Yann Ylavic wrote:
> On Tue, Mar 2, 2021 at 1:37 AM <yl...@apache.org> wrote:
>>
>> Author: ylavic
>> Date: Tue Mar 2 00:37:18 2021
>> New Revision: 1887060
>>
>> URL: http://svn.apache.org/viewvc?rev=1887060&view=rev
>> Log:
>> Align apr_mmap()ing offset to a page boundary. PR 65158.
> []
>> --- apr/apr/trunk/include/apr_mmap.h (original)
>> +++ apr/apr/trunk/include/apr_mmap.h Tue Mar 2 00:37:18 2021
>> @@ -62,11 +62,10 @@ typedef struct apr_mmap_t apr
>> struct apr_mmap_t {
>> /** The pool the mmap structure was allocated out of. */
>> apr_pool_t *cntxt;
>> -#ifdef BEOS
>> +#if defined(BEOS)
>> /** An area ID. Only valid on BeOS */
>> area_id area;
>> -#endif
>> -#ifdef WIN32
>> +#elif defined(WIN32)
>> /** The handle of the file mapping */
>> HANDLE mhandle;
>> /** The start of the real memory page area (mapped view) */
>> @@ -75,6 +74,8 @@ struct apr_mmap_t {
>> apr_off_t pstart;
>> apr_size_t psize;
>> apr_off_t poffset;
>> +#else
>> + apr_off_t poffset;
>> #endif
>> /** The start of the memory mapped area */
>> void *mm;
>
> By adding this "poffset" member to apr_mmap_t (on unixes), its layout changes.
> Can I backport this to 1.7 or should I find another way to store
> "poffset" there (like some pool/cntxt userdata)?
I don't think that this can be backported this way as it changes a non opaque struct on the public API.
So you would need to look for a different area to store this data.
Regards
Rüdiger
Re: svn commit: r1887060 - in /apr/apr/trunk: CHANGES
include/apr_mmap.h mmap/unix/mmap.c test/data/mmap_large_datafile.txt test/testmmap.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Mar 2, 2021 at 1:37 AM <yl...@apache.org> wrote:
>
> Author: ylavic
> Date: Tue Mar 2 00:37:18 2021
> New Revision: 1887060
>
> URL: http://svn.apache.org/viewvc?rev=1887060&view=rev
> Log:
> Align apr_mmap()ing offset to a page boundary. PR 65158.
[]
> --- apr/apr/trunk/include/apr_mmap.h (original)
> +++ apr/apr/trunk/include/apr_mmap.h Tue Mar 2 00:37:18 2021
> @@ -62,11 +62,10 @@ typedef struct apr_mmap_t apr
> struct apr_mmap_t {
> /** The pool the mmap structure was allocated out of. */
> apr_pool_t *cntxt;
> -#ifdef BEOS
> +#if defined(BEOS)
> /** An area ID. Only valid on BeOS */
> area_id area;
> -#endif
> -#ifdef WIN32
> +#elif defined(WIN32)
> /** The handle of the file mapping */
> HANDLE mhandle;
> /** The start of the real memory page area (mapped view) */
> @@ -75,6 +74,8 @@ struct apr_mmap_t {
> apr_off_t pstart;
> apr_size_t psize;
> apr_off_t poffset;
> +#else
> + apr_off_t poffset;
> #endif
> /** The start of the memory mapped area */
> void *mm;
By adding this "poffset" member to apr_mmap_t (on unixes), its layout changes.
Can I backport this to 1.7 or should I find another way to store
"poffset" there (like some pool/cntxt userdata)?
Regards;
Yann.