This change does introduce an API change, and a somewhat possibly dangerous structure size alteration, and seems to be out of bounds for apr 1.7.x branch, as identified in the commit message.
Anyone using a build crossing runtimes between apr 1.7.0 and 1.7.1 may experience unintended side-effects. The eventual change still is present for apr 1.8 adopters. Thoughts? ylavic are you willing to revert? On Thu, Mar 11, 2021 at 10:21 AM <yla...@apache.org> wrote: > > Author: ylavic > Date: Thu Mar 11 16:21:07 2021 > New Revision: 1887497 > > URL: http://svn.apache.org/viewvc?rev=1887497&view=rev > Log: > Merge r1887060 from trunk: > > 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() now deletes the full range > from the start of the page. > > Tests updated accordingly. > > > * Changes between r1887060 and this 1.7.x backport > > Since the layout of struct apr_mmap_t cannot change (non opaque), the poffset > is over-allocated in mmap/unix/mmap.cmmap.c which uses this layout internally: > struct mm_layout { > apr_mmap_t mm; > apr_off_t poffset; > }; > > This works for all the apr_mmap_t created by apr_mmap_create() with which any > apr_mmap_*() function can then be called. > > If a user creates an apr_mmap_t "by hand", with this change it cannot be > passed > to apr_mmap_dup() without causing an out-of-bound read. This is hardly a new > limitation though, the refcounting which is the point of apr_mmap_dup() and > apr_mmap_delete() comes from the private function mmap_cleanup() anyway, so > there is no point in using these two functions with hand made apr_mmap_t (if > there ever was a point in the first place to create an hand made > apr_mmap_t..). > > > Submitted by: ylavic > > Added: > apr/apr/branches/1.7.x/test/data/mmap_large_datafile.txt > - copied unchanged from r1887060, > apr/apr/trunk/test/data/mmap_large_datafile.txt > Modified: > apr/apr/branches/1.7.x/ (props changed) > apr/apr/branches/1.7.x/CHANGES > apr/apr/branches/1.7.x/mmap/unix/mmap.c > apr/apr/branches/1.7.x/test/testfnmatch.c > apr/apr/branches/1.7.x/test/testmmap.c > > Propchange: apr/apr/branches/1.7.x/ > ------------------------------------------------------------------------------ > Merged /apr/apr/trunk:r1887060 > > Modified: apr/apr/branches/1.7.x/CHANGES > URL: > http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/CHANGES?rev=1887497&r1=1887496&r2=1887497&view=diff > ============================================================================== > --- apr/apr/branches/1.7.x/CHANGES [utf-8] (original) > +++ apr/apr/branches/1.7.x/CHANGES [utf-8] Thu Mar 11 16:21:07 2021 > @@ -1,6 +1,8 @@ > -*- coding: utf-8 -*- > Changes for APR 1.7.1 > > + *) Align apr_mmap()ing offset to a page boundary. PR 65158. [Yann Ylavic] > + > *) Don't silently set APR_FOPEN_NOCLEANUP for apr_file_mktemp() created > file > to avoid a fd and inode leak when/if later passed to > apr_file_setaside(). > [Yann Ylavic] > > Modified: apr/apr/branches/1.7.x/mmap/unix/mmap.c > URL: > http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/mmap/unix/mmap.c?rev=1887497&r1=1887496&r2=1887497&view=diff > ============================================================================== > --- apr/apr/branches/1.7.x/mmap/unix/mmap.c (original) > +++ apr/apr/branches/1.7.x/mmap/unix/mmap.c Thu Mar 11 16:21:07 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 > @@ -42,6 +47,34 @@ > > #if APR_HAS_MMAP || defined(BEOS) > > +#ifndef BEOS > +struct mm_layout { > + apr_mmap_t mm; > + apr_off_t poffset; > +}; > + > +static APR_INLINE > +apr_mmap_t *alloc_with_poffset(apr_pool_t *p) > +{ > + struct mm_layout *layout = apr_pcalloc(p, sizeof(*layout)); > + return &layout->mm; > +} > + > +static APR_INLINE > +void set_poffset(apr_mmap_t *mm, apr_off_t poffset) > +{ > + struct mm_layout *layout = (struct mm_layout *)mm; > + layout->poffset = poffset; > +} > + > +static APR_INLINE > +apr_off_t get_poffset(apr_mmap_t *mm) > +{ > + struct mm_layout *layout = (struct mm_layout *)mm; > + return layout->poffset; > +} > +#endif > + > static apr_status_t mmap_cleanup(void *themmap) > { > apr_mmap_t *mm = themmap; > @@ -61,7 +94,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 - get_poffset(mm), mm->size + > get_poffset(mm)); > #endif > mm->mm = (void *)-1; > > @@ -81,6 +114,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 > > @@ -97,9 +132,11 @@ APR_DECLARE(apr_status_t) apr_mmap_creat > > if (file == NULL || file->filedes == -1 || file->buffered) > return APR_EBADF; > - (*new) = (apr_mmap_t *)apr_pcalloc(cont, sizeof(apr_mmap_t)); > - > + > #ifdef BEOS > + > + *new = (apr_mmap_t *)apr_pcalloc(cont, sizeof(apr_mmap_t)); > + > /* XXX: mmap shouldn't really change the seek offset */ > apr_file_seek(file, APR_SET, &offset); > > @@ -121,7 +158,9 @@ APR_DECLARE(apr_status_t) apr_mmap_creat > read(file->filedes, mm, size); > > (*new)->area = aid; > + > #else > + *new = alloc_with_poffset(cont); > > if (flag & APR_MMAP_WRITE) { > native_flags |= PROT_WRITE; > @@ -130,7 +169,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 +189,8 @@ APR_DECLARE(apr_status_t) apr_mmap_creat > } > #endif > > - (*new)->mm = mm; > + (*new)->mm = (char *)mm + poffset; > + set_poffset(*new, poffset); > (*new)->size = size; > (*new)->cntxt = cont; > APR_RING_ELEM_INIT(*new, link); > @@ -154,7 +205,12 @@ APR_DECLARE(apr_status_t) apr_mmap_dup(a > apr_mmap_t *old_mmap, > apr_pool_t *p) > { > +#ifdef BEOS > *new_mmap = (apr_mmap_t *)apr_pmemdup(p, old_mmap, sizeof(apr_mmap_t)); > +#else > + *new_mmap = (apr_mmap_t *)apr_pmemdup(p, old_mmap, > + sizeof(struct mm_layout)); > +#endif > (*new_mmap)->cntxt = p; > > APR_RING_INSERT_AFTER(old_mmap, *new_mmap, link); > > Modified: apr/apr/branches/1.7.x/test/testfnmatch.c > URL: > http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/test/testfnmatch.c?rev=1887497&r1=1887496&r2=1887497&view=diff > ============================================================================== > --- apr/apr/branches/1.7.x/test/testfnmatch.c (original) > +++ apr/apr/branches/1.7.x/test/testfnmatch.c Thu Mar 11 16:21:07 2021 > @@ -23,7 +23,7 @@ > * .txt extension in the data directory at the time testfnmatch > * happens to be run (!?!). */ > > -#define NUM_FILES (5) > +#define NUM_FILES (6) > > #define APR_FNM_BITS 15 > #define APR_FNM_FAILBIT 256 > > Modified: apr/apr/branches/1.7.x/test/testmmap.c > URL: > http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/test/testmmap.c?rev=1887497&r1=1887496&r2=1887497&view=diff > ============================================================================== > --- apr/apr/branches/1.7.x/test/testmmap.c (original) > +++ apr/apr/branches/1.7.x/test/testmmap.c Thu Mar 11 16:21:07 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,24 +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_UREAD | > APR_GREAD, p); > + rv = apr_file_open(&thefile, file1, APR_FOPEN_READ, > + APR_UREAD | APR_GREAD, ptest); > ABTS_INT_EQUAL(tc, APR_SUCCESS, rv); > ABTS_PTR_NOTNULL(tc, thefile); > } > @@ -95,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) > @@ -126,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) > @@ -138,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) > @@ -147,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 > >
