[PATCH] Throw an exception when mutating read-only data

[PATCH] Throw an exception when mutating read-only data

[Andy Wingo]

* libguile/init.c (scm_i_init_guile): Install the SIGSEGV handler unless
  GUILE_INSTALL_SIGSEGV_HANDLER is 0.
* libguile/loader.c
  (scm_maybe_throw_exception_for_mutation_of_read_only_data): New public
  function.
  (sigsegv_handler): New helper.
  (scm_install_sigsegv_handler): New public function.
* libguile/loader.h: Declare new API.
---
 libguile/init.c   |  7 +++++
 libguile/loader.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libguile/loader.h |  3 +++
 3 files changed, 87 insertions(+)

diff --git a/libguile/init.c b/libguile/init.c
index b046685d4..b333e7def 100644
--- a/libguile/init.c
+++ b/libguile/init.c
@@ -530,6 +530,13 @@ scm_i_init_guile (void *base)
   scm_init_rw ();
   scm_init_extensions ();
 
+  if (scm_getenv_int ("GUILE_INSTALL_SIGSEGV_HANDLER", 1))
+    {
+      if (scm_install_sigsegv_handler () != 0)
+        perror ("failed to install SIGSEGV handler");
+      /* Continue regardless.  */
+    }
+
   atexit (cleanup_for_exit);
   scm_load_startup_files ();
   scm_init_load_should_auto_compile ();
diff --git a/libguile/loader.c b/libguile/loader.c
index 7b1adc9c9..d12137be3 100644
--- a/libguile/loader.c
+++ b/libguile/loader.c
@@ -745,6 +745,83 @@ scm_all_mapped_elf_images (void)
   return result;
 }
 
+void
+scm_maybe_throw_exception_for_mutation_of_read_only_data (void *addr)
+{
+  if (!find_mapped_elf_image_unlocked (addr))
+    return;
+
+  /* Assume that a SEGV originating from access to an address in our
+     mapped ELF images is because that part of the image was mapped
+     read-only, and user code is trying to mutate it.  Throw an
+     exception instead.  */
+  scm_misc_error (NULL, "Attempt to mutate read-only value", SCM_EOL);
+}
+
+static struct sigaction prev_sigsegv_handler;
+
+static void
+sigsegv_handler (int sig, siginfo_t *si, void *unused)
+{
+  scm_maybe_throw_exception_for_mutation_of_read_only_data (si->si_addr);
+
+  /* If we got here, we need to chain up to the previously installed
+     handler.  */
+  if (prev_sigsegv_handler.sa_flags & SA_SIGINFO)
+    {
+      if (prev_sigsegv_handler.sa_sigaction == SIG_IGN)
+        /* Although it's not meaningful to continue after ignoring
+           SIGSEGV, that's exactly what the user requested, so just
+           return.  */
+        return;
+
+      if (prev_sigsegv_handler.sa_sigaction != SIG_DFL)
+        {
+          prev_sigsegv_handler.sa_sigaction (sig, si, unused);
+          /* The meaning of a program is undefined after returning from
+             SIGSEGV, but that's what the previous handler did; respect
+             its judgment.  */
+          return;
+        }
+    }
+  else
+    {
+      if (prev_sigsegv_handler.sa_handler == SIG_IGN)
+        /* See note above.  */
+        return;
+
+      if (prev_sigsegv_handler.sa_handler != SIG_DFL)
+        {
+          prev_sigsegv_handler.sa_handler (sig);
+          /* See note above.  */
+          return;
+        }
+    }
+
+  /* If we got here, the previous handler was SIG_DFL which will abort
+     the program.  Restore SIG_DFL and re-raise the signal.  */
+  {
+    struct sigaction sa;
+    sigemptyset (&sa.sa_mask);
+    sa.sa_handler = SIG_DFL;
+    if (sigaction (sig, &sa, NULL) != 0)
+      abort ();
+    raise (sig);
+    /* Unreachable.  */
+    abort ();
+  }
+}
+
+int
+scm_install_sigsegv_handler (void)
+{
+  struct sigaction sa;
+  sa.sa_flags = SA_SIGINFO;
+  sigemptyset (&sa.sa_mask);
+  sa.sa_sigaction = sigsegv_handler;
+  return sigaction (SIGSEGV, &sa, &prev_sigsegv_handler);
+}
+
 struct frame_map_prefix
 {
   scm_t_uint32 text_offset;
diff --git a/libguile/loader.h b/libguile/loader.h
index 5c719cbce..f097a6ccc 100644
--- a/libguile/loader.h
+++ b/libguile/loader.h
@@ -27,6 +27,9 @@ SCM_API SCM scm_load_thunk_from_memory (SCM bv);
 SCM_INTERNAL const scm_t_uint8 *
 scm_find_slot_map_unlocked (const scm_t_uint32 *ip);
 
+SCM_API int scm_install_sigsegv_handler (void);
+SCM_API void scm_maybe_throw_exception_for_mutation_of_read_only_data (void *);
+
 SCM_INTERNAL void scm_bootstrap_loader (void);
 SCM_INTERNAL void scm_init_loader (void);
 
-- 
2.12.2

Re: [PATCH] Throw an exception when mutating read-only data

[Derek Upham]

Andy Wingo <wingo@pobox.com> writes:

> * libguile/init.c (scm_i_init_guile): Install the SIGSEGV handler unless
>   GUILE_INSTALL_SIGSEGV_HANDLER is 0.
> * libguile/loader.c
>   (scm_maybe_throw_exception_for_mutation_of_read_only_data): New public
>   function.
>   (sigsegv_handler): New helper.
>   (scm_install_sigsegv_handler): New public function.
> * libguile/loader.h: Declare new API.

That doesn’t support systems without HAVE_SIGACTION.  Do we assume HAVE_SIGACTION for all systems these days?

The handler setup code is outside of scmsig.c, so there will be conflicts with the save/restore mechanisms.  For example, scm_restore_signals won’t re-establish that handler.

Derek

-- 
Derek Upham
sand@blarg.net

Re: [PATCH] Throw an exception when mutating read-only data

[Pedro Alves]

Hi Andy,

I'm mostly clueless on Guile's internals, but this patch caught my
eye, given the potential for conflict with GDBs use of libguile.

On 04/02/2017 11:11 AM, Andy Wingo wrote:

> +void
> +scm_maybe_throw_exception_for_mutation_of_read_only_data (void *addr)
> +{
> +  if (!find_mapped_elf_image_unlocked (addr))
> +    return;
> +
> +  /* Assume that a SEGV originating from access to an address in our
> +     mapped ELF images is because that part of the image was mapped
> +     read-only, and user code is trying to mutate it.  Throw an
> +     exception instead.  */
> +  scm_misc_error (NULL, "Attempt to mutate read-only value", SCM_EOL);

AFAICS, guile uses bare setjmp/longjmp for exceptions.

But, one should not use longjmp to jump out of a signal handler, since
that leaves the signal mask disabled if the system automatically masks
the signal on handler entry (which is true on e.g., the Linux kernel).
Instead, sigsetjmp+siglongjmp should be used, in order to restore
the signal mask.

Since sigsetjmp is heavy weight (it requires a system call to get the current
mask [1]), I'd recommend instead to use sigsetjmp/siglongjmp around the
problematic code area, and then call scm_misc_error back in mainline code (as
opposed to changing SCM_I_SETJMP/SCM_I_LONGJMP and forcing all exception
handling to use sigsetjmp+siglongjmp).

[1] - See <https://sourceware.org/ml/gdb-patches/2016-04/msg00249.html>
and the linked URL for a practical difference that made in GDB.

Thanks,
Pedro Alves

Re: [PATCH] Throw an exception when mutating read-only data

[Andy Wingo]

Hi :)

On Sun 02 Apr 2017 17:34, Derek Upham <sand@blarg.net> writes:

> Andy Wingo <wingo@pobox.com> writes:
>
>> * libguile/init.c (scm_i_init_guile): Install the SIGSEGV handler unless
>>   GUILE_INSTALL_SIGSEGV_HANDLER is 0.
>> * libguile/loader.c
>>   (scm_maybe_throw_exception_for_mutation_of_read_only_data): New public
>>   function.
>>   (sigsegv_handler): New helper.
>>   (scm_install_sigsegv_handler): New public function.
>> * libguile/loader.h: Declare new API.
>
> That doesn’t support systems without HAVE_SIGACTION.  Do we assume
> HAVE_SIGACTION for all systems these days?

I think we use Gnulib to ensure that sigaction is available, at least as
a wrapper.  On the other hand apparently that doesn't ensure that
SA_SIGINFO is available on mingw :/  Too bad.

If we find a way to make this work reliably on POSIX I guess we could
have it work there but not yet on mingw.

> The handler setup code is outside of scmsig.c, so there will be
> conflicts with the save/restore mechanisms.  For example,
> scm_restore_signals won’t re-establish that handler.

Good point!  It seems this patch is but half-baked :)

Andy

Re: [PATCH] Throw an exception when mutating read-only data

[Andy Wingo]

Hi Pedro,

On Mon 03 Apr 2017 12:49, Pedro Alves <palves@redhat.com> writes:

> AFAICS, guile uses bare setjmp/longjmp for exceptions.
>
> But, one should not use longjmp to jump out of a signal handler, since
> that leaves the signal mask disabled if the system automatically masks
> the signal on handler entry (which is true on e.g., the Linux kernel).
> Instead, sigsetjmp+siglongjmp should be used, in order to restore
> the signal mask.
>
> Since sigsetjmp is heavy weight (it requires a system call to get the current
> mask [1]), I'd recommend instead to use sigsetjmp/siglongjmp around the
> problematic code area, and then call scm_misc_error back in mainline code (as
> opposed to changing SCM_I_SETJMP/SCM_I_LONGJMP and forcing all exception
> handling to use sigsetjmp+siglongjmp).
>
> [1] - See <https://sourceware.org/ml/gdb-patches/2016-04/msg00249.html>
> and the linked URL for a practical difference that made in GDB.

Thanks very much for this information!  I was unaware of how this
worked.  This is the first SIGSEGV handler I have written :)

In Guile the details are similar as in GDB's try/catch, though with some
slight differences.  Currently the usual way Guile does things is to
SCM_I_SETJMP only once before entering the bytecode interpreter.
Establishing a "catch" within Scheme code re-uses that jump buffer.
Currently the "throw" code will unwind the VM state and then do a
SCM_I_LONGJMP back to the jump buffer that corresponds to that catch,
which will re-enter the bytecode interpreter with the new VM state.
Multiple "catch" instantiations can share the same jump buffer.
Probably at some point in the future we optimize things so that if the
"throw" happens from bytecode, within the same VM invocation as the
"catch", that we can avoid the SCM_I_LONGJMP on abort as well.

All of this applies only to Scheme code though.  If you have C code that
throws an error, it will always SCM_I_LONGJMP; and if you call into the
VM from C or establish a "catch" from C, it will SCM_I_SETJMP before
entering the VM.  So in that situation we'd be seeing the performance
impact for signal mask things.

I guess the conclusion here would be that we would need to measure
performance a bit before making a change like this.

Andy