[PATCH v8 3/6] shm: use workaround if mremap is not available

[Pekka Paalanen]

On Wed, 27 Feb 2019 21:13:10 +0200
Leonid Bobrov <mazocomp at disroot.org> wrote:

> From: Imre Vadász <ivadasz at dragonflybsd.org>
> 
> Signed-off-by: Leonid Bobrov <mazocomp at disroot.org>
> ---
>  configure.ac      |  4 ++++
>  src/wayland-shm.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)

Hi Leonid,

this is almost a good patch. I'd probably like the MAP_ANON thing as a
separate patch though, since it doesn't seem to be related to the
mremap usage. More below.

> 
> diff --git a/configure.ac b/configure.ac
> index c0f1c37..dcefc78 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -65,6 +65,10 @@ AC_SUBST(GCC_CFLAGS)
>  AC_CHECK_HEADERS([sys/prctl.h])
>  AC_CHECK_FUNCS([accept4 mkostemp posix_fallocate prctl])
>  
> +# mremap() and sys/mman.h are needed for the shm
> +AC_CHECK_FUNCS([mremap])
> +AC_CHECK_HEADERS([sys/mman.h])
> +
>  # waitid() and signal.h are needed for the test suite.
>  AC_CHECK_FUNCS([waitid])
>  AC_CHECK_HEADERS([signal.h])
> diff --git a/src/wayland-shm.c b/src/wayland-shm.c
> index 4191231..5bfdece 100644
> --- a/src/wayland-shm.c
> +++ b/src/wayland-shm.c
> @@ -40,6 +40,9 @@

sys/mman.h is checked in configure.ac, but the result is not used.

>  #include <assert.h>
>  #include <signal.h>
>  #include <pthread.h>

> +#ifdef HAVE_SYS_PARAM_H

Was there something to set this in configure.ac? I didn't see it.

> +#include <sys/param.h>
> +#endif
>  
>  #include "wayland-util.h"
>  #include "wayland-private.h"
> @@ -84,7 +87,24 @@ shm_pool_finish_resize(struct wl_shm_pool *pool)
>  	if (pool->size == pool->new_size)
>  		return;
>  
> +#ifdef HAVE_MREMAP
>  	data = mremap(pool->data, pool->size, pool->new_size, MREMAP_MAYMOVE);
> +#else
> +	int32_t osize = (pool->size + PAGE_SIZE - 1) & ~PAGE_MASK;

We try to not mix code and declarations, i.e. osize would need to be
declared at the start of the function or at least at the start of a
block.

> +	if (pool->new_size <= osize) {
> +		pool->size = pool->new_size;
> +		return;
> +	}
> +	data = mmap(pool->data + osize, pool->new_size - osize, PROT_READ,
> +		    MAP_SHARED | MAP_TRYFIXED, pool->fd, osize);
> +	if (data == MAP_FAILED) {
> +		munmap(pool->data, pool->size);
> +		data = mmap(NULL, pool->new_size, PROT_READ, MAP_SHARED, pool->fd, 0);
> +	} else {
> +		pool->size = pool->new_size;
> +		return;
> +	}

Because we already reject shrinking in shm_pool_resize(), this
implementation looks correct to me.

> +#endif
>  	if (data == MAP_FAILED) {

The failure path here seems to leave the old munmapped address in
pool->data which would cause it to be munmapped again when the pool is
destroyed. I also don't think munmap(MAP_FAILED, ...) would be a no-op,
so would need to protect against that as well.

>  		wl_resource_post_error(pool->resource,
>  				       WL_SHM_ERROR_INVALID_FD,
> @@ -495,6 +515,7 @@ sigbus_handler(int signum, siginfo_t *info, void *context)
>  	sigbus_data->fallback_mapping_used = 1;
>  
>  	/* This should replace the previous mapping */
> +#ifdef HAVE_MREMAP
>  	if (mmap(pool->data, pool->size,
>  		 PROT_READ | PROT_WRITE,
>  		 MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS,
> @@ -502,6 +523,15 @@ sigbus_handler(int signum, siginfo_t *info, void *context)
>  		reraise_sigbus();
>  		return;
>  	}
> +#else
> +	if (mmap(pool->data, pool->size,
> +		 PROT_READ,
> +		 MAP_PRIVATE | MAP_FIXED | MAP_ANON,
> +		 0, 0) == MAP_FAILED) {
> +		reraise_sigbus();
> +		return;
> +	}
> +#endif

The only difference here seems to be MAP_ANONYMOUS vs. MAP_ANON. Why
would that be related to the existence of mremap()?

My Linux manual says that MAP_ANON is a deprecated synonym for
MAP_ANONYMOUS. How about

#ifndef MAP_ANONYMOUS
#define MAP_ANONYMOUS MAP_ANON
#endif

instead of this hunk?


Thanks,
pq

>  }
>  
>  static void
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20190311/45bd2b79/attachment.sig>