[PATCH 2/2] wayland-cursor: Introduce os-fault-handler

[Pekka Paalanen]

On Fri, 1 Feb 2013 18:17:58 +0100
Martin Minarik <minarik11 at student.fiit.stuba.sk> wrote:

> This fault handler can be used for recovery from an out of memory
> condition (OOM). When writing to a full mmap'ed memory, the SIGBUS
> signal causes the process to terminate.
> 
> It can be caught and the process may decide what to do next,
> for example restart the computation later, or shut down and
> tell the user.

Hi Martin,

I do not think this is a good idea. A shared library messing up with
the process signal handlers is a recipe for pain for all users of the
library. If you do this, then the application cannot handle SIGBUS
anymore the way all developers would expect. In fact, the problem is
even more escalated, since libwayland-cursor may be used by another
library, and the application developers may not even know they are
using libwayland-cursor, or depending on build time or even runtime
setups libwayland-cursor might get used or not.

Signals are also very thread-dangerous: signals are process-global, and
to ensure signal handling in a certain thread, threads must explicitly
block signals, IIRC.

In my opinion, this is far more trouble for every single application
than it is worth. It would force all direct and indirect users of this
library to go far out of their way to just account for the fact that
libwayland-cursor might be handling SIGBUS.

Applications just need to implement their own SIGBUS handling or use
a toolkit that abstracts it all, if they want to recover. I don't think
there is any other realistic option.


Thanks,
pq

> ---
>  cursor/Makefile.am        |    2 +
>  cursor/os-fault-handler.c |  143 +++++++++++++++++++++++++++++++++++++++++++++
>  cursor/os-fault-handler.h |   11 ++++
>  cursor/wayland-cursor.c   |   17 ++++++
>  4 files changed, 173 insertions(+)
>  create mode 100644 cursor/os-fault-handler.c
>  create mode 100644 cursor/os-fault-handler.h
> 
> diff --git a/cursor/Makefile.am b/cursor/Makefile.am
> index 61029b5..59885f0 100644
> --- a/cursor/Makefile.am
> +++ b/cursor/Makefile.am
> @@ -6,6 +6,8 @@ libwayland_cursor_la_SOURCES =			\
>  	wayland-cursor.c			\
>  	os-compatibility.c			\
>  	os-compatibility.h			\
> +	os-fault-handler.c			\
> +	os-fault-handler.h			\
>  	cursor-data.h				\
>  	xcursor.c				\
>  	xcursor.h
> diff --git a/cursor/os-fault-handler.c b/cursor/os-fault-handler.c
> new file mode 100644
> index 0000000..437f80e
> --- /dev/null
> +++ b/cursor/os-fault-handler.c
> @@ -0,0 +1,143 @@
> +/*
> + * Copyright © 2013 Martin Minarik
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and its
> + * documentation for any purpose is hereby granted without fee, provided that
> + * the above copyright notice appear in all copies and that both that copyright
> + * notice and this permission notice appear in supporting documentation, and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission.  The copyright holders make no representations
> + * about the suitability of this software for any purpose.  It is provided "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +#include <signal.h>
> +#include <sys/mman.h>
> +#include <fcntl.h>
> +
> +#include "os-fault-handler.h"
> +
> +volatile sig_atomic_t fault_handler_safe;
> +volatile sig_atomic_t fault_handler_zerofd;
> +void* fault_handler_last_dummy_page;
> +const size_t pagesize = 0x1000;
> +
> +void fault_handler_try_free_dummy_page()
> +{
> +	if (fault_handler_last_dummy_page != NULL)
> +		munmap(fault_handler_last_dummy_page, pagesize);
> +	fault_handler_last_dummy_page = NULL;
> +}
> +
> +void
> +fault_action(int sig, siginfo_t *siginfo, void *ptr)
> +{
> +	void *addr = (unsigned char *)(siginfo->si_addr);
> +	int perrno = errno; /* Save the previous errno. */
> +
> +	if (siginfo->si_code != BUS_ADRERR){
> +		goto fail;
> +	}
> +
> +	/* Tell the client that the fault occured. */
> +	if (fault_handler_safe == 1) {
> +		fault_handler_safe = 0;
> +	}
> +
> +	fault_handler_try_free_dummy_page();
> +
> +	/* Needs to be aligned to lower page boundary. */
> +	fault_handler_last_dummy_page =
> +		(void *)((uintptr_t)addr & (~(pagesize - 1)));
> +
> +	/* Mmap dummy zeroes to the requested address. */
> +	fault_handler_last_dummy_page = mmap(addr, pagesize,
> +		PROT_READ | PROT_WRITE,
> +		MAP_PRIVATE | MAP_FIXED, fault_handler_zerofd, 0);
> +
> +	if (fault_handler_last_dummy_page == MAP_FAILED) {
> +		fault_handler_last_dummy_page = NULL;
> +		goto fail;
> +	}
> +
> +	/* Resume the program. */
> +	errno = perrno;
> +	return;
> +fail:
> +	errno = perrno;
> +	abort();
> +}
> +
> +/* Must call only once. */
> +int fault_handler_init()
> +{
> +	fault_handler_last_dummy_page = NULL;
> +	fault_handler_safe = 1;
> +
> +	int fd = open("/dev/zero", O_RDWR);
> +	if (fd < 0)
> +		return -1;
> +
> +	fault_handler_zerofd = fd;
> +
> +	struct sigaction act;
> +	sigemptyset(&act.sa_mask);
> +	act.sa_flags = SA_RESTART | SA_SIGINFO;
> +	act.sa_sigaction = fault_action;
> +	sigaction(SIGBUS, &act, NULL);
> +
> +	return 0;
> +}
> +
> +void *fault_handler_detected()
> +{
> +	void *addr;
> +	if (mem_is())
> +		addr = NULL;
> +	else
> +		addr = fault_handler_last_dummy_page;
> +	fault_handler_destroy();
> +	return addr;
> +}
> +
> +/* Must fault_handler_destroy() manually after using this. */
> +void *fault_handler_detected_continue()
> +{
> +	if (mem_is()){
> +		return NULL;
> +	}
> +	void *addr = fault_handler_last_dummy_page;
> +	fault_handler_try_free_dummy_page();
> +	fault_handler_safe = 1;
> +	return addr;
> +}
> +
> +int fault_handler_destroy()
> +{
> +	fault_handler_try_free_dummy_page();
> +	fault_handler_safe = 1;
> +	close(fault_handler_zerofd);
> +	return signal(SIGBUS, SIG_DFL);
> +}
> +
> +/*
> + * Have memory? No fault occured?
> + * 1 == OK. 0 == FAULT.
> +*/
> +inline
> +int mem_is()
> +{
> +	return fault_handler_safe;
> +}
> diff --git a/cursor/os-fault-handler.h b/cursor/os-fault-handler.h
> new file mode 100644
> index 0000000..c33be14
> --- /dev/null
> +++ b/cursor/os-fault-handler.h
> @@ -0,0 +1,11 @@
> +#ifndef _FAULT_HANDLER_H_
> +#define _FAULT_HANDLER_H_
> +
> +inline int mem_is();
> +
> +int fault_handler_init();
> +void *fault_handler_detected();
> +void *fault_handler_detected_continue();
> +int fault_handler_destroy();
> +
> +#endif // _FAULT_HANDLER_H_
> diff --git a/cursor/wayland-cursor.c b/cursor/wayland-cursor.c
> index 821ef31..f5c5830 100644
> --- a/cursor/wayland-cursor.c
> +++ b/cursor/wayland-cursor.c
> @@ -30,6 +30,7 @@
>  #include <sys/mman.h>
>  
>  #include "os-compatibility.h"
> +#include "os-fault-handler.h"
>  
>  #define ARRAY_LENGTH(a) (sizeof (a) / sizeof (a)[0])
>  
> @@ -242,9 +243,16 @@ wl_cursor_create_from_data(struct cursor_metadata *metadata,
>  	if (shm_pool_allocate(theme->pool, size, &image->offset)) {
>  		goto err;
>  	}
> +
> +	fault_handler_init();
> +
>  	memcpy(theme->pool->data + image->offset,
>  	       cursor_data + metadata->offset, size);
>  
> +	if (fault_handler_detected()){
> +		goto err;
> +	}
> +
>  	return &cursor->cursor;
>  
>  err:
> @@ -293,6 +301,8 @@ wl_cursor_create_from_xcursor_images(XcursorImages *images,
>  	cursor->cursor.name = strdup(images->name);
>  	cursor->total_delay = 0;
>  
> +	fault_handler_init();
> +
>  	for (i = 0; i < images->nimage; i++) {
>  		image = malloc(sizeof *image);
>  		cursor->cursor.images[i] = (struct wl_cursor_image *) image;
> @@ -315,6 +325,9 @@ wl_cursor_create_from_xcursor_images(XcursorImages *images,
>  			memcpy(theme->pool->data + image->offset,
>  			       images->images[i]->pixels, size);
>  
> +			if (fault_handler_detected_continue()){
> +				fail = -1;
> +			}
>  		}
>  		if (fail == -1) {
>  			/* on error, rollback the allocations. */
> @@ -323,10 +336,14 @@ wl_cursor_create_from_xcursor_images(XcursorImages *images,
>  				free(cursor->cursor.images[j]);
>  			}
>  
> +			fault_handler_destroy();
> +
>  			goto err;
>  		}
>  	}
>  
> +	fault_handler_destroy();
> +
>  	return &cursor->cursor;
>  err:
>  	free(cursor);