Two issues with stack overflow protection

Two issues with stack overflow protection

[Eli Zaretskii]

The implementation of stack_overflow on sysdep.c was recently changed
so as not to use sys/resource.h and getrlimit, but configure.ac still
insists on these two features in order to include the recovery code,
which I think should be fixed.

More importantly, the recovery simply longjmps to command_loop,
whereas similar features like Fthrow and Fsignal do much more in
unwind_to_catch.  Shouldn't stack overflow recovery do that as well?
Otherwise, the specpdl stack, byte_stack_list, lisp_eval_depth
etc. all stay at their values they had at stack overflow time, no?

Re: Two issues with stack overflow protection

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]

Eli Zaretskii wrote:
> The implementation of stack_overflow on sysdep.c was recently changed
> so as not to use sys/resource.h and getrlimit, but configure.ac still
> insists on these two features in order to include the recovery code,
> which I think should be fixed.

Thanks, fixed with the attached patch.

> More importantly, the recovery simply longjmps to command_loop,
> whereas similar features like Fthrow and Fsignal do much more in
> unwind_to_catch.  Shouldn't stack overflow recovery do that as well?
> Otherwise, the specpdl stack, byte_stack_list, lisp_eval_depth
> etc. all stay at their values they had at stack overflow time, no?

No, they are cleared back to top-level values, because when command_loop's call 
to sigsetjmp returns nonzero, it then calls init_eval and this resets them.

There is a problem that the speclpdl stack's unwind-protect and 
dynamic-let-bindings are unceremoniously discarded on stack overflow.  I suppose 
that should get fixed, though it may be a bit tricky to avoid looping.

[-- Attachment #2: 0001-Remove-unnecessary-stack-overflow-dependency.patch --]
[-- Type: text/x-diff, Size: 1163 bytes --]

From 2377572a880400182cd370c5ca104c7151e0f20c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 28 Jul 2015 21:41:59 -0700
Subject: [PATCH] Remove unnecessary stack overflow dependency

* configure.ac (HAVE_STACK_OVERFLOW_HANDLING):
Don't worry about $ac_cv_header_sys_resource_h and
$ac_cv_func_getrlimit, as they're no longer needed for this.
Problem reported by Eli Zaretskii in:
http://lists.gnu.org/archive/html/emacs-devel/2015-07/msg00443.html
---
 configure.ac | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index 19b8b9d..45008d8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4557,9 +4557,7 @@ if test $emacs_cv_func_sigsetjmp = yes; then
 fi
 
 # We need all of these features to handle C stack overflows.
-if test "$ac_cv_header_sys_resource_h" = "yes" &&
-   test "$ac_cv_func_getrlimit" = "yes" &&
-   test "$emacs_cv_func_sigsetjmp" = "yes" &&
+if test "$emacs_cv_func_sigsetjmp" = "yes" &&
    test "$emacs_cv_alternate_stack" = yes; then
   AC_DEFINE([HAVE_STACK_OVERFLOW_HANDLING], 1,
     [Define to 1 if C stack overflow can be handled in some cases.])
-- 
2.1.0

Re: Two issues with stack overflow protection

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

On 07/28/2015 10:01 PM, Paul Eggert wrote:
> Eli Zaretskii wrote:
>> The implementation of stack_overflow on sysdep.c was recently changed
>> so as not to use sys/resource.h and getrlimit, but configure.ac still
>> insists on these two features in order to include the recovery code,
>> which I think should be fixed.
> 
> Thanks, fixed with the attached patch.
> 
>> More importantly, the recovery simply longjmps to command_loop,
>> whereas similar features like Fthrow and Fsignal do much more in
>> unwind_to_catch.  Shouldn't stack overflow recovery do that as well?
>> Otherwise, the specpdl stack, byte_stack_list, lisp_eval_depth
>> etc. all stay at their values they had at stack overflow time, no?
> 
> No, they are cleared back to top-level values, because when
> command_loop's call to sigsetjmp returns nonzero, it then calls
> init_eval and this resets them.
> 
> There is a problem that the speclpdl stack's unwind-protect and
> dynamic-let-bindings are unceremoniously discarded on stack overflow.  I
> suppose that should get fixed, though it may be a bit tricky to avoid
> looping.

What's wrong with just mprotecting a guard page at the end of the stack,
and on overflow, giving that region normal protection, unwinding as
normal, then, at top level, restoring the guard page?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Two issues with stack overflow protection

[Paul Eggert]

Daniel Colascione wrote:
> What's wrong with just mprotecting a guard page at the end of the stack,
> and on overflow, giving that region normal protection, unwinding as
> normal, then, at top level, restoring the guard page?

Unwinding can grow the stack.

Re: Two issues with stack overflow protection

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]

On 07/29/2015 12:06 AM, Paul Eggert wrote:
> Daniel Colascione wrote:
>> What's wrong with just mprotecting a guard page at the end of the stack,
>> and on overflow, giving that region normal protection, unwinding as
>> normal, then, at top level, restoring the guard page?
> 
> Unwinding can grow the stack.

Sure. That's why you open up more stack to do the unwinding. Having done
that, if you still overflow, just abort. At that point, you can't
guarantee correct program semantics.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Two issues with stack overflow protection

[Paul Eggert]

Daniel Colascione wrote:
>>> What's wrong with just mprotecting a guard page at the end of the stack,
>>> >>and on overflow, giving that region normal protection, unwinding as
>>> >>normal, then, at top level, restoring the guard page?
>> >
>> >Unwinding can grow the stack.
> Sure. That's why you open up more stack to do the unwinding. Having done
> that, if you still overflow, just abort.

Yes, that was my point.  Emacs already does the business about the guard page, 
and opening up more stack, and so forth.  The tricky part is the "if you still 
overflow, just abort", something that's easy enough to describe at the high 
level but perhaps not so easy to actually write the code.  Part of the issue is 
that the guard page business is done under the covers by the OS, not by Emacs 
directly -- in general Emacs doesn't know where the guard page(s) are.

I'm sure there are other issues that won't get discovered until someone actually 
writes and tests something.  For example, here's something I just thought of: 
the conservative marking phase of the Emacs garbage collector may need to be 
taught about the split stack (currently it assumes the C stack is contiguous).

Re: Two issues with stack overflow protection

[Eli Zaretskii]

> Date: Wed, 29 Jul 2015 06:18:17 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> Daniel Colascione wrote:
> >>> What's wrong with just mprotecting a guard page at the end of the stack,
> >>> >>and on overflow, giving that region normal protection, unwinding as
> >>> >>normal, then, at top level, restoring the guard page?
> >> >
> >> >Unwinding can grow the stack.
> > Sure. That's why you open up more stack to do the unwinding. Having done
> > that, if you still overflow, just abort.
> 
> Yes, that was my point.  Emacs already does the business about the guard page, 
> and opening up more stack, and so forth.  The tricky part is the "if you still 
> overflow, just abort", something that's easy enough to describe at the high 
> level but perhaps not so easy to actually write the code.  Part of the issue is 
> that the guard page business is done under the covers by the OS, not by Emacs 
> directly -- in general Emacs doesn't know where the guard page(s) are.

Maybe I'm missing something, but none of the data structures involved
in "normal" throw to top-level are on the stack, right?  So why cannot
we call the equivalent of (top-level) _after_ we sig_longjmp out of
the signal handler?

Re: Two issues with stack overflow protection

[Paul Eggert]

Eli Zaretskii wrote:
> none of the data structures involved
> in "normal" throw to top-level are on the stack, right?

No, byte_stack_list points into the C stack.