Re: new snapshot available: grep-3.4-almost.26-5419

Re: new snapshot available: grep-3.4-almost.26-5419

[Paul Eggert]

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

In <https://lists.gnu.org/r/grep-devel/2020-09/msg00052.html> on 9/18/20 2:00 
PM, Bruce Dubbs wrote:
> ... if I run ./configure --prefix=/usr --bindir=/bin grep wants to link with 
> /usr/lib/libsigsegv.so /usr/lib/libc.a instead of just -lsigsegv
> In this later case, grep segfaults early.
> 
> If I remove /usr/lib/libc.a, everything is fine.  I do not know why configure 
> wants to use a static libc if the destination libraries are specified.
> 
> Is this possibly a gnulib issue?

Yes, most likely it's induced by grep's use of Gnulib's libsigsegv module. I'll 
cc. this email to the Gnulib mailing list.

To help narrow this down, let's try to reproduce the problem in Gnulib, without 
having grep be involved. I created the attached tarball from Gnulib master with 
the commands:

./gnulib-tool --create-testdir --dir c-stack-test c-stack
tar czf c-stack-test.tgz c-stack-test

Please run the following commands on your end and see whether they cause a 
similar failure:

tar xf c-stack-test.tgz
cd c-stack-test
./configure --prefix=/usr --bindir=/bin
make check

I ran that on my Ubuntu 18.04.5 x86-64 platform and got output that contained this:

...
checking for working C stack overflow detection... yes
checking for correct stack_t interpretation... yes
checking for precise C stack overflow detection... no
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for shared library run path origin... done
checking 32-bit host C ABI... no
checking for ELF binary format... yes
checking for the common suffixes of directories in the library search path... 
lib,lib,lib
checking for libsigsegv... yes
checking how to link with libsigsegv... -lsigsegv
...

and all 93 tests succeeded.  Here are a few more things I tried and where I got 
the expected results; please compare them to your platform too.

$ ldd gltests/test-c-stack
	linux-vdso.so.1 (0x00007ffcf55e9000)
	libsigsegv.so.2 => /usr/lib/x86_64-linux-gnu/libsigsegv.so.2 (0x00007f28136f9000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f2813308000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f2813b04000)
$ gltests/test-c-stack
test-c-stack: stack overflow
$ gltests/test-c-stack 1
test-c-stack: program error
Aborted (core dumped)

[-- Attachment #2: c-stack-test.tgz --]
[-- Type: application/x-compressed-tar, Size: 836712 bytes --]

Re: new snapshot available: grep-3.4-almost.26-5419

[Bruce Dubbs]

On 9/18/20 5:16 PM, Paul Eggert wrote:
> In <https://lists.gnu.org/r/grep-devel/2020-09/msg00052.html> on 9/18/20 
> 2:00 PM, Bruce Dubbs wrote:
>> ... if I run ./configure --prefix=/usr --bindir=/bin grep wants to 
>> link with /usr/lib/libsigsegv.so /usr/lib/libc.a instead of just 
>> -lsigsegv
>> In this later case, grep segfaults early.
>>
>> If I remove /usr/lib/libc.a, everything is fine.  I do not know why 
>> configure wants to use a static libc if the destination libraries are 
>> specified.
>>
>> Is this possibly a gnulib issue?
> 
> Yes, most likely it's induced by grep's use of Gnulib's libsigsegv 
> module. I'll cc. this email to the Gnulib mailing list.
> 
> To help narrow this down, let's try to reproduce the problem in Gnulib, 
> without having grep be involved. I created the attached tarball from 
> Gnulib master with the commands:
> 
> ./gnulib-tool --create-testdir --dir c-stack-test c-stack
> tar czf c-stack-test.tgz c-stack-test
> 
> Please run the following commands on your end and see whether they cause 
> a similar failure:
> 
> tar xf c-stack-test.tgz
> cd c-stack-test
> ./configure --prefix=/usr --bindir=/bin
> make check
> 
> I ran that on my Ubuntu 18.04.5 x86-64 platform and got output that 
> contained this:
> 
> ...
> checking for working C stack overflow detection... yes
> checking for correct stack_t interpretation... yes
> checking for precise C stack overflow detection... no
> checking for ld used by gcc... /usr/bin/ld
> checking if the linker (/usr/bin/ld) is GNU ld... yes
> checking for shared library run path origin... done
> checking 32-bit host C ABI... no
> checking for ELF binary format... yes
> checking for the common suffixes of directories in the library search 
> path... lib,lib,lib
> checking for libsigsegv... yes
> checking how to link with libsigsegv... -lsigsegv

I get:

checking for libsigsegv... yes
checking how to link with libsigsegv... /usr/lib/libsigsegv.so 
/usr/lib/libc.a


Do you have /usr/lib/libc.a ?

I'll note that we do 'strip --strip-debug' on libc.a


> and all 93 tests succeeded.  


    dummy 0: ./test-suite.log
===============================

# TOTAL: 93
# PASS:  91
# SKIP:  0
# XFAIL: 0
# FAIL:  2
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: test-c-stack.sh
=====================

./test-c-stack.sh: line 7: 18000 Segmentation fault      ${CHECKER} 
./test-c-stack${EXEEXT} 2> t-c-stack.tmp
FAIL test-c-stack.sh (exit status: 1)

FAIL: test-c-stack2.sh
======================

FAIL test-c-stack2.sh (exit status: 1)

> Here are a few more things I tried and 
> where I got the expected results; please compare them to your platform too.
> 
> $ ldd gltests/test-c-stack
>      linux-vdso.so.1 (0x00007ffcf55e9000)
>      libsigsegv.so.2 => /usr/lib/x86_64-linux-gnu/libsigsegv.so.2 
> (0x00007f28136f9000)
>      libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f2813308000)
>      /lib64/ld-linux-x86-64.so.2 (0x00007f2813b04000)

$ ldd gltests/test-c-stack
         linux-vdso.so.1 (0x00007ffd62100000)
         libsigsegv.so.2 => /usr/lib/libsigsegv.so.2 (0x00007f3d0f843000)
         libgcc_s.so.1 => /usr/lib/libgcc_s.so.1 (0x00007f3d0f829000)
         libc.so.6 => /lib/libc.so.6 (0x00007f3d0f663000)
         /lib64/ld-linux-x86-64.so.2 (0x00007f3d0f86f000

> $ gltests/test-c-stack
> test-c-stack: stack overflow

$ gltests/test-c-stack
Segmentation fault

> $ gltests/test-c-stack 1
> test-c-stack: program error
> Aborted (core dumped)

Segmentation fault

   -- Bruce

Re: libsigsegv on LinuxFromScratch

[Bruno Haible]

Bruce Dubbs wrote in
<https://lists.gnu.org/r/grep-devel/2020-09/msg00052.html>
<https://lists.gnu.org/r/grep-devel/2020-09/msg00053.html> :

> > checking for libsigsegv... yes
> > checking how to link with libsigsegv... -lsigsegv
> 
> I get:
> 
> checking for libsigsegv... yes
> checking how to link with libsigsegv... /usr/lib/libsigsegv.so 
> /usr/lib/libc.a

Is this really a multiline string (also in the value of LIBSIGSEGV in
config.status)? Or are these space-separated values?

In the latter case, is linking with libc.a normally working?

Also, if there is a file /usr/lib/libsigsegv.la, can you please show
its contents?

Bruno

Re: libsigsegv on LinuxFromScratch

[Bruce Dubbs]

On 9/18/20 6:24 PM, Bruno Haible wrote:
> Bruce Dubbs wrote in
> <https://lists.gnu.org/r/grep-devel/2020-09/msg00052.html>
> <https://lists.gnu.org/r/grep-devel/2020-09/msg00053.html> :
> 
>>> checking for libsigsegv... yes
>>> checking how to link with libsigsegv... -lsigsegv
>>
>> I get:
>>
>> checking for libsigsegv... yes
>> checking how to link with libsigsegv... /usr/lib/libsigsegv.so
>> /usr/lib/libc.a
> 
> Is this really a multiline string (also in the value of LIBSIGSEGV in
> .sconfigtatus)? Or are these space-separated values?

It's email wrapping.  It is one line in the log.

$ grep LIBSIGSEGV config.status
S["LIBSIGSEGV_PREFIX"]="/usr"
S["LTLIBSIGSEGV"]="-L/usr/lib -lsigsegv -lc"
S["LIBSIGSEGV"]="/usr/lib/libsigsegv.so -lc"
S["HAVE_LIBSIGSEGV"]="yes"
D["HAVE_LIBSIGSEGV"]=" 1"
D["HAVE_LIBSIGSEGV"]=" 1"


> In the latter case, is linking with libc.a normally working?

No it is not.

> Also, if there is a file /usr/lib/libsigsegv.la, can you please show
> its contents?

# libsigsegv.la - a libtool library file
# Generated by libtool (GNU libtool) 2.4.6
#
# Please DO NOT delete this file!
# It is necessary for linking the library.

# The name that we can dlopen(3).
dlname='libsigsegv.so.2'

# Names of this library.
library_names='libsigsegv.so.2.0.5 libsigsegv.so.2 libsigsegv.so'

# The name of the static archive.
old_library=''

# Linker flags that cannot go in dependency_libs.
inherited_linker_flags=''

# Libraries that this one depends upon.
dependency_libs=' -lc'

# Names of additional weak libraries provided by this library
weak_library_names=''

# Version information for libsigsegv.
current=2
age=0
revision=5

# Is this an already installed library?
installed=yes

# Should we warn about portability when linking against -modules?
shouldnotlink=no

# Files to dlopen/dlpreopen
dlopen=''
dlpreopen=''

# Directory that this library needs to be installed in:
libdir='/usr/lib'

----------

But I think the problem is libc.a, not libsigsegv.


One additional note: adding --prefix=/usr to configure causes the 
problem.  If I only add --bindir=/bin, then the problem does not occur.

   -- Bruce

Re: libsigsegv on LinuxFromScratch

[Bruce Dubbs]

On 9/18/20 6:47 PM, Bruce Dubbs wrote:
> On 9/18/20 6:24 PM, Bruno Haible wrote:
>> Bruce Dubbs wrote in
>> <https://lists.gnu.org/r/grep-devel/2020-09/msg00052.html>
>> <https://lists.gnu.org/r/grep-devel/2020-09/msg00053.html> :
>>
>>>> checking for libsigsegv... yes
>>>> checking how to link with libsigsegv... -lsigsegv
>>>
>>> I get:
>>>
>>> checking for libsigsegv... yes
>>> checking how to link with libsigsegv... /usr/lib/libsigsegv.so
>>> /usr/lib/libc.a

Here is some background.  When we build grep on LFS, we normally do not 
have either pcre or libsegsev installed.  It has built fine for years. 
We have had no complaints.

In the latest grep-3.4-almost.31-fe6c I have been testing after both 
pcre and libsigsevv have been.  I haven't been installing the new grep 
versions, only going through make check.

Here is what I found.

If ./configure is run without options, the build and checks are fine.

If ./configure is run with --prefix=/usr with both libsigsegv and 
/usr/lib/libc.a are present, configure gives:

configure:10063: checking for libsigsegv
configure:10086: gcc -o conftest -g -O2   conftest.c 
/usr/lib/libsigsegv.so /usr/lib/libc.a  >&5
configure:10086: $? = 0
configure:10097: result: yes
configure:10104: checking how to link with libsigsegv
configure:10106: result: /usr/lib/libsigsegv.so /usr/lib/libc.a

Which causes grep to segfault immediately.

If ./configure is run with --prefix=/usr AND --without-libsigsegv-prefix
the result is:

configure:10063: checking for libsigsegv
configure:10086: gcc -o conftest -g -O2   conftest.c  -lsigsegv >&5
configure:10086: $? = 0
configure:10097: result: yes
configure:10104: checking how to link with libsigsegv
configure:10106: result: -lsigsegv

And the results of the checks are as expected.

If I move all libtool .la files (I really dislike libtool) out of the 
way, then things are fine again with just --prefix=/usr:

configure:10063: checking for libsigsegv
configure:10086: gcc -o conftest -g -O2   conftest.c 
/usr/lib/libsigsegv.so  >&5
configure:10086: $? = 0
configure:10097: result: yes
configure:10104: checking how to link with libsigsegv
configure:10106: result: /usr/lib/libsigsegv.so

So the bottom line is there is something interacting between libtool and 
teh default libsigsegv procedures, however none of the .la files 
references libc.a directly.

I hope this helps.

   -- Bruce

Re: libsigsegv on LinuxFromScratch

[Bruno Haible]

Thanks Bruce for the details about the .la file.

> If ./configure is run with --prefix=/usr with both libsigsegv and 
> /usr/lib/libc.a are present, configure gives:
> 
> configure:10063: checking for libsigsegv
> configure:10086: gcc -o conftest -g -O2   conftest.c 
> /usr/lib/libsigsegv.so /usr/lib/libc.a  >&5
> configure:10086: $? = 0
> configure:10097: result: yes
> configure:10104: checking how to link with libsigsegv
> configure:10106: result: /usr/lib/libsigsegv.so /usr/lib/libc.a
> 
> Which causes grep to segfault immediately.

Whereas on an Ubuntu 16.04 system the effect is weird in a different sense:
the compilation fails with an error
  /usr/bin/ld: dynamic STT_GNU_IFUNC symbol `strcmp' with pointer equality in `/usr/lib/libc.a(strcmp.o)' can not be used when making an executable; recompile with -fPIE and relink with -pie
leading to
  checking for libsigsegv... no

> If I move all libtool .la files (I really dislike libtool) out of the 
> way

The .la file contains a comment

  # Please DO NOT delete this file!
  # It is necessary for linking the library.

but in fact all distros I know of don't distribute .la files. Go figure.

The patch below should fix it.


2020-09-19  Bruno Haible  <bruno@clisp.org>

	havelib: Avoid linking with libc.a on GNU systems.
	Reported by Bruce Dubbs <bruce.dubbs@gmail.com> in
	<https://lists.gnu.org/archive/html/grep-devel/2020-09/msg00052.html>.
	* m4/lib-link.m4 (AC_LIB_LINKFLAGS_BODY): When processing the
	dependency_libs value of a .la file, ignore '-lc' options on GNU
	systems.

diff --git a/m4/lib-link.m4 b/m4/lib-link.m4
index eecf70e..5ce1aa2 100644
--- a/m4/lib-link.m4
+++ b/m4/lib-link.m4
@@ -1,4 +1,4 @@
-# lib-link.m4 serial 31
+# lib-link.m4 serial 32
 dnl Copyright (C) 2001-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -631,7 +631,20 @@ AC_DEFUN([AC_LIB_LINKFLAGS_BODY],
                     ;;
                   -l*)
                     dnl Handle this in the next round.
-                    names_next_round="$names_next_round "`echo "X$dep" | sed -e 's/^X-l//'`
+                    dnl But on GNU systems, ignore -lc options, because
+                    dnl   - linking with libc is the default anyway,
+                    dnl   - linking with libc.a may produce an error
+                    dnl     "/usr/bin/ld: dynamic STT_GNU_IFUNC symbol `strcmp' with pointer equality in `/usr/lib/libc.a(strcmp.o)' can not be used when making an executable; recompile with -fPIE and relink with -pie"
+                    dnl     or may produce an executable that always crashes, see
+                    dnl     <https://lists.gnu.org/archive/html/grep-devel/2020-09/msg00052.html>.
+                    dep=`echo "X$dep" | sed -e 's/^X-l//'`
+                    if test "X$dep" != Xc \
+                       || case $host_os in
+                            linux* | gnu* | k*bsd*-gnu) false ;;
+                            *)                          true ;;
+                          esac; then
+                      names_next_round="$names_next_round $dep"
+                    fi
                     ;;
                   *.la)
                     dnl Handle this in the next round. Throw away the .la's

Re: libsigsegv on LinuxFromScratch

[Bruce Dubbs]

On 9/19/20 6:47 PM, Bruno Haible wrote:
> Thanks Bruce for the details about the .la file.
> 
>> If ./configure is run with --prefix=/usr with both libsigsegv and
>> /usr/lib/libc.a are present, configure gives:
>>
>> configure:10063: checking for libsigsegv
>> configure:10086: gcc -o conftest -g -O2   conftest.c
>> /usr/lib/libsigsegv.so /usr/lib/libc.a  >&5
>> configure:10086: $? = 0
>> configure:10097: result: yes
>> configure:10104: checking how to link with libsigsegv
>> configure:10106: result: /usr/lib/libsigsegv.so /usr/lib/libc.a
>>
>> Which causes grep to segfault immediately.
> 
> Whereas on an Ubuntu 16.04 system the effect is weird in a different sense:
> the compilation fails with an error
>    /usr/bin/ld: dynamic STT_GNU_IFUNC symbol `strcmp' with pointer equality in `/usr/lib/libc.a(strcmp.o)' can not be used when making an executable; recompile with -fPIE and relink with -pie
> leading to
>    checking for libsigsegv... no
> 
>> If I move all libtool .la files (I really dislike libtool) out of the
>> way
> 
> The .la file contains a comment
> 
>    # Please DO NOT delete this file!
>    # It is necessary for linking the library.
> 
> but in fact all distros I know of don't distribute .la files. Go figure.
> 
> The patch below should fix it.
> 
> 
> 2020-09-19  Bruno Haible  <bruno@clisp.org>
> 
> 	havelib: Avoid linking with libc.a on GNU systems.
> 	Reported by Bruce Dubbs <bruce.dubbs@gmail.com> in
> 	<https://lists.gnu.org/archive/html/grep-devel/2020-09/msg00052.html>.
> 	* m4/lib-link.m4 (AC_LIB_LINKFLAGS_BODY): When processing the
> 	dependency_libs value of a .la file, ignore '-lc' options on GNU
> 	systems.
> 
> diff --git a/m4/lib-link.m4 b/m4/lib-link.m4
> index eecf70e..5ce1aa2 100644
> --- a/m4/lib-link.m4
> +++ b/m4/lib-link.m4
> @@ -1,4 +1,4 @@
> -# lib-link.m4 serial 31
> +# lib-link.m4 serial 32
>   dnl Copyright (C) 2001-2020 Free Software Foundation, Inc.
>   dnl This file is free software; the Free Software Foundation
>   dnl gives unlimited permission to copy and/or distribute it,
> @@ -631,7 +631,20 @@ AC_DEFUN([AC_LIB_LINKFLAGS_BODY],
>                       ;;
>                     -l*)
>                       dnl Handle this in the next round.
> -                    names_next_round="$names_next_round "`echo "X$dep" | sed -e 's/^X-l//'`
> +                    dnl But on GNU systems, ignore -lc options, because
> +                    dnl   - linking with libc is the default anyway,
> +                    dnl   - linking with libc.a may produce an error
> +                    dnl     "/usr/bin/ld: dynamic STT_GNU_IFUNC symbol `strcmp' with pointer equality in `/usr/lib/libc.a(strcmp.o)' can not be used when making an executable; recompile with -fPIE and relink with -pie"
> +                    dnl     or may produce an executable that always crashes, see
> +                    dnl     <https://lists.gnu.org/archive/html/grep-devel/2020-09/msg00052.html>.
> +                    dep=`echo "X$dep" | sed -e 's/^X-l//'`
> +                    if test "X$dep" != Xc \
> +                       || case $host_os in
> +                            linux* | gnu* | k*bsd*-gnu) false ;;
> +                            *)                          true ;;
> +                          esac; then
> +                      names_next_round="$names_next_round $dep"
> +                    fi
>                       ;;
>                     *.la)
>                       dnl Handle this in the next round. Throw away the .la's
> 

Wow.  I'm glad you found where to fix the problem.  Thanks.

I know some systems need libtool, but Linux tools can find what they 
need without it.   In some cases .la file are embedded in pkgconfig 
files and removing the .la files sometimes needs fixup.

Just part of the joys of building from source.

   -- Bruce

Re: libsigsegv on LinuxFromScratch

[Paul Eggert]

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

On 9/19/20 4:47 PM, Bruno Haible wrote:

> 	havelib: Avoid linking with libc.a on GNU systems.

Thanks for fixing the bug. This caused me to look at the c-stack module for the 
first time in a while, and I found some old-fashioned code and some unlikely 
bugs and fixed one misfeature when libsigsegv is not in use. I installed the 
attached patches to the c-stack module in Gnulib to try to fix it. These changes 
shouldn't affect how c-stack behaves when libsigsegv is in use.

While looking into this I discovered pthread_getattr_np + pthread_attr_getstack 
which might have been nice for the GNU/Linux part of c-stack.c, except they're 
not async-signal-safe. As I understand it, libsigsegv works around the 
async-signal-safe problem by parsing /proc/self/maps with async-signal-safe 
functions, which is quite a feat and is probably beyond what c-stack should do.

PS. I also found this circa-2015 Linux kernel bug related to PIE that looks like 
it might be of interest to the libsigsegv developers

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-1000253

This bug causes /proc/self/maps to report the wrong VMA (actually, overlapping 
VMAs) for the stack. This could be worth a comment in the libsigsegv sources. 
For more commentary in this area please see:

https://stackoverflow.com/questions/56893353/analyzing-memory-mapping-of-a-process-with-pmap-stack/56920770

PPS. Given the longstanding security problems with stack overflow (as witness 
the name stackoverflow.com!) it is somewhat disturbing that there is still no 
reliable way in GNU/Linux to answer the simple question "Where's my stack?" or 
to detect and recover from stack overflow reliably. What's up with that?

[-- Attachment #2: 0001-c-stack-improve-checking-if-libsigsegv.patch --]
[-- Type: text/x-patch, Size: 18794 bytes --]

From 8ba9126d00bfe1ab77a5c820c58c68933d4df85c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 20 Sep 2020 11:48:17 -0700
Subject: [PATCH 1/2] c-stack: improve checking if !libsigsegv
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If SIGINFO_WORKS, do not treat a null pointer dereference as if it
were a stack overflow.  Use uintptr_t and INT_ADD_WRAPV to avoid
unlikely pointer overflow.  Also, fix some obsolete code and typos.
I found these problems while looking into this bug report:
https://lists.gnu.org/r/grep-devel/2020-09/msg00053.html
* lib/c-stack.c: Include c-stack.h first, to test interface.
Include inttypes.h for UINTPTR_MAX, stdbool.h, stddef.h for
max_align_t, intprops.h for INT_ADD_WRAPV.
(USE_LIBSIGSEGV): New macro; use it to simplify later code.
(SIGSTKSZ): Simplify setup.  Work around libsigsegv bug only
for libsigsegv 2.8 and earlier since the bug should be fixed
after that.
(alternate_signal_stack): Use max_align_t instead of doing it by hand.
(segv_handler, overflow_handler, segv_handler) [DEBUG]:
Assume sprintf returns byte count; this assumption is safe now.
(page_size): New static volatile variable, since sysconf isn’t
documented to be async-signal-safe on Solaris.  This variable is
present and used if (!USE_LIBSIGSEGV && HAVE_SIGALTSTACK &&
HAVE_DECL_SIGALTSTACK && HAVE_STACK_OVERFLOW_HANDLING &&
SIGINFO_WORKS).
(segv_handler): Use it if present.  Never report null pointer
dereference as a stack overflow.  Check for (unlikely) unsigned
and/or pointer overflow.
* m4/c-stack.m4 (AC_SYS_XSI_STACK_OVERFLOW_HEURISTIC):
Rename cache variables to gl_cv_sys_stack_overflow_works
and gl_cv_sys_xsi_stack_overflow_heuristic.
All uses changed.
(gl_PREREQ_C_STACK): Do not require AC_FUNC_ALLOCA, since
c-stack no longer uses STACK_DIRECTION.
Do not check for unistd.h, since we depend on unistd.
Fix shell typo ‘$"ac_cv_sys_xsi_stack_overflow_heuristic"’.
* modules/c-stack (Depends-on): Sort.  Add intprops, inttypes,
stdbool, stddef.
---
 ChangeLog       |  37 +++++++++++++
 lib/c-stack.c   | 135 ++++++++++++++++++++++++++++--------------------
 m4/c-stack.m4   |  33 ++++++------
 modules/c-stack |  12 +++--
 4 files changed, 139 insertions(+), 78 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index cc1cd4a40..087b9232f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,40 @@
+2020-09-20  Paul Eggert  <eggert@cs.ucla.edu>
+
+	c-stack: improve checking if !libsigsegv
+	If SIGINFO_WORKS, do not treat a null pointer dereference as if it
+	were a stack overflow.  Use uintptr_t and INT_ADD_WRAPV to avoid
+	unlikely pointer overflow.  Also, fix some obsolete code and typos.
+	I found these problems while looking into this bug report:
+	https://lists.gnu.org/r/grep-devel/2020-09/msg00053.html
+	* lib/c-stack.c: Include c-stack.h first, to test interface.
+	Include inttypes.h for UINTPTR_MAX, stdbool.h, stddef.h for
+	max_align_t, intprops.h for INT_ADD_WRAPV.
+	(USE_LIBSIGSEGV): New macro; use it to simplify later code.
+	(SIGSTKSZ): Simplify setup.  Work around libsigsegv bug only
+	for libsigsegv 2.8 and earlier since the bug should be fixed
+	after that.
+	(alternate_signal_stack): Use max_align_t instead of doing it by hand.
+	(segv_handler, overflow_handler, segv_handler) [DEBUG]:
+	Assume sprintf returns byte count; this assumption is safe now.
+	(page_size): New static volatile variable, since sysconf isn’t
+	documented to be async-signal-safe on Solaris.  This variable is
+	present and used if (!USE_LIBSIGSEGV && HAVE_SIGALTSTACK &&
+	HAVE_DECL_SIGALTSTACK && HAVE_STACK_OVERFLOW_HANDLING &&
+	SIGINFO_WORKS).
+	(segv_handler): Use it if present.  Never report null pointer
+	dereference as a stack overflow.  Check for (unlikely) unsigned
+	and/or pointer overflow.
+	* m4/c-stack.m4 (AC_SYS_XSI_STACK_OVERFLOW_HEURISTIC):
+	Rename cache variables to gl_cv_sys_stack_overflow_works
+	and gl_cv_sys_xsi_stack_overflow_heuristic.
+	All uses changed.
+	(gl_PREREQ_C_STACK): Do not require AC_FUNC_ALLOCA, since
+	c-stack no longer uses STACK_DIRECTION.
+	Do not check for unistd.h, since we depend on unistd.
+	Fix shell typo ‘$"ac_cv_sys_xsi_stack_overflow_heuristic"’.
+	* modules/c-stack (Depends-on): Sort.  Add intprops, inttypes,
+	stdbool, stddef.
+
 2020-09-20  Bruno Haible  <bruno@clisp.org>
 
 	Revert now-unnecessary override of config.guess on Alpine Linux 3.10.
diff --git a/lib/c-stack.c b/lib/c-stack.c
index 59606299b..3649c1bfe 100644
--- a/lib/c-stack.c
+++ b/lib/c-stack.c
@@ -35,30 +35,25 @@
 
 #include <config.h>
 
+#include "c-stack.h"
+
 #include "gettext.h"
 #define _(msgid) gettext (msgid)
 
 #include <errno.h>
+#include <inttypes.h>
 
 #include <signal.h>
 #if ! HAVE_STACK_T && ! defined stack_t
 typedef struct sigaltstack stack_t;
 #endif
-#ifndef SIGSTKSZ
-# define SIGSTKSZ 16384
-#elif HAVE_LIBSIGSEGV && SIGSTKSZ < 16384
-/* libsigsegv 2.6 through 2.8 have a bug where some architectures use
-   more than the Linux default of an 8k alternate stack when deciding
-   if a fault was caused by stack overflow.  */
-# undef SIGSTKSZ
-# define SIGSTKSZ 16384
-#endif
 
+#include <stdbool.h>
+#include <stddef.h>
 #include <stdlib.h>
 #include <string.h>
 
-/* Posix 2001 declares ucontext_t in <ucontext.h>, Posix 200x in
-   <signal.h>.  */
+/* Pre-2008 POSIX declared ucontext_t in ucontext.h instead of signal.h.  */
 #if HAVE_UCONTEXT_H
 # include <ucontext.h>
 #endif
@@ -69,13 +64,26 @@ typedef struct sigaltstack stack_t;
 # include <stdio.h>
 #endif
 
-#if HAVE_LIBSIGSEGV
+/* Use libsigsegv only if needed; kernels like Solaris can detect
+   stack overflow without the overhead of an external library.  */
+#define USE_LIBSIGSEGV (HAVE_XSI_STACK_OVERFLOW_HEURISTIC && HAVE_LIBSIGSEGV)
+
+#if USE_LIBSIGSEGV
 # include <sigsegv.h>
+/* libsigsegv 2.6 through 2.8 have a bug where some architectures use
+   more than the Linux default of an 8k alternate stack when deciding
+   if a fault was caused by stack overflow.  */
+# if LIBSIGSEGV_VERSION <= 0x0208 && SIGSTKSZ < 16384
+#  undef SIGSTKSZ
+# endif
+#endif
+#ifndef SIGSTKSZ
+# define SIGSTKSZ 16384
 #endif
 
-#include "c-stack.h"
 #include "exitfail.h"
 #include "ignore-value.h"
+#include "intprops.h"
 #include "getprogname.h"
 
 #if defined SA_ONSTACK && defined SA_SIGINFO
@@ -97,7 +105,7 @@ static _GL_ASYNC_SAFE void (* volatile segv_action) (int);
 static char const * volatile program_error_message;
 static char const * volatile stack_overflow_message;
 
-#if ((HAVE_LIBSIGSEGV && ! HAVE_XSI_STACK_OVERFLOW_HEURISTIC) \
+#if (USE_LIBSIGSEGV                                           \
      || (HAVE_SIGALTSTACK && HAVE_DECL_SIGALTSTACK            \
          && HAVE_STACK_OVERFLOW_HANDLING))
 
@@ -111,12 +119,12 @@ static _GL_ASYNC_SAFE _Noreturn void
 die (int signo)
 {
   char const *message;
-#if !SIGINFO_WORKS && !HAVE_LIBSIGSEGV
+# if !SIGINFO_WORKS && !USE_LIBSIGSEGV
   /* We can't easily determine whether it is a stack overflow; so
      assume that the rest of our program is perfect (!) and that
      this segmentation violation is a stack overflow.  */
   signo = 0;
-#endif /* !SIGINFO_WORKS && !HAVE_LIBSIGSEGV */
+# endif
   segv_action (signo);
   message = signo ? program_error_message : stack_overflow_message;
   ignore_value (write (STDERR_FILENO, progname, strlen (progname)));
@@ -128,22 +136,12 @@ die (int signo)
   raise (signo);
   abort ();
 }
-#endif
-
-#if (HAVE_SIGALTSTACK && HAVE_DECL_SIGALTSTACK \
-     && HAVE_STACK_OVERFLOW_HANDLING) || HAVE_LIBSIGSEGV
 
 /* Storage for the alternate signal stack.  */
 static union
 {
   char buffer[SIGSTKSZ];
-
-  /* These other members are for proper alignment.  There's no
-     standard way to guarantee stack alignment, but this seems enough
-     in practice.  */
-  long double ld;
-  long l;
-  void *p;
+  max_align_t align;
 } alternate_signal_stack;
 
 static _GL_ASYNC_SAFE void
@@ -153,10 +151,7 @@ null_action (int signo _GL_UNUSED)
 
 #endif /* SIGALTSTACK || LIBSIGSEGV */
 
-/* Only use libsigsegv if we need it; platforms like Solaris can
-   detect stack overflow without the overhead of an external
-   library.  */
-#if HAVE_LIBSIGSEGV && ! HAVE_XSI_STACK_OVERFLOW_HEURISTIC
+#if USE_LIBSIGSEGV
 
 /* Nonzero if general segv handler could not be installed.  */
 static volatile int segv_handler_missing;
@@ -171,8 +166,8 @@ segv_handler (void *address _GL_UNUSED, int serious)
   {
     char buf[1024];
     int saved_errno = errno;
-    sprintf (buf, "segv_handler serious=%d\n", serious);
-    ignore_value (write (STDERR_FILENO, buf, strlen (buf)));
+    ignore_value (write (STDERR_FILENO, buf,
+                         sprintf (buf, "segv_handler serious=%d\n", serious)));
     errno = saved_errno;
   }
 # endif
@@ -193,9 +188,10 @@ overflow_handler (int emergency, stackoverflow_context_t context _GL_UNUSED)
 # if DEBUG
   {
     char buf[1024];
-    sprintf (buf, "overflow_handler emergency=%d segv_handler_missing=%d\n",
-             emergency, segv_handler_missing);
-    ignore_value (write (STDERR_FILENO, buf, strlen (buf)));
+    ignore_value (write (STDERR_FILENO, buf,
+                         sprintf (buf, ("overflow_handler emergency=%d"
+                                        " segv_handler_missing=%d\n"),
+                                  emergency, segv_handler_missing)));
   }
 # endif
 
@@ -228,6 +224,8 @@ c_stack_action (_GL_ASYNC_SAFE void (*action) (int))
 
 # if SIGINFO_WORKS
 
+static size_t volatile page_size;
+
 /* Handle a segmentation violation and exit.  This function is
    async-signal-safe.  */
 
@@ -235,8 +233,17 @@ static _GL_ASYNC_SAFE _Noreturn void
 segv_handler (int signo, siginfo_t *info, void *context _GL_UNUSED)
 {
   /* Clear SIGNO if it seems to have been a stack overflow.  */
-#  if ! HAVE_XSI_STACK_OVERFLOW_HEURISTIC
-  /* We can't easily determine whether it is a stack overflow; so
+
+  /* If si_code is nonpositive, something like raise (SIGSEGV) occurred
+     so it cannot be a stack overflow.  */
+  bool cannot_be_stack_overflow = info->si_code <= 0;
+
+  /* An unaligned address cannot be a stack overflow.  */
+#  if FAULT_YIELDS_SIGBUS
+  cannot_be_stack_overflow |= signo == SIGBUS && info->si_code == BUS_ADRALN;
+#  endif
+
+  /* If we can't easily determine that it is not a stack overflow,
      assume that the rest of our program is perfect (!) and that
      this segmentation violation is a stack overflow.
 
@@ -246,33 +253,44 @@ segv_handler (int signo, siginfo_t *info, void *context _GL_UNUSED)
      Solaris populates uc_stack with the details of the
      interrupted stack, while Linux populates it with the details
      of the current stack.  */
-  signo = 0;
-#  else
-  if (0 < info->si_code)
+  if (!cannot_be_stack_overflow)
     {
       /* If the faulting address is within the stack, or within one
          page of the stack, assume that it is a stack overflow.  */
+      uintptr_t faulting_address = (uintptr_t) info->si_addr;
+
+      /* On all platforms we know of, the first page is not in the
+         stack to catch null pointer dereferening.  However, all other
+         pages might be in the stack.  */
+      void *stack_base = (void *) (uintptr_t) page_size;
+      uintptr_t stack_size = 0; stack_size -= page_size;
+#  if HAVE_XSI_STACK_OVERFLOW_HEURISTIC
+      /* Tighten the stack bounds via the XSI heuristic.  */
       ucontext_t const *user_context = context;
-      char const *stack_base = user_context->uc_stack.ss_sp;
-      size_t stack_size = user_context->uc_stack.ss_size;
-      char const *faulting_address = info->si_addr;
-      size_t page_size = sysconf (_SC_PAGESIZE);
-      size_t s = faulting_address - stack_base + page_size;
-      if (s < stack_size + 2 * page_size)
+      stack_base = user_context->uc_stack.ss_sp;
+      stack_size = user_context->uc_stack.ss_size;
+#  endif
+      uintptr_t base = (uintptr_t) stack_base,
+        lo = (INT_SUBTRACT_WRAPV (base, page_size, &lo) || lo < page_size
+              ? page_size : lo),
+        hi = ((INT_ADD_WRAPV (base, stack_size, &hi)
+               || INT_ADD_WRAPV (hi, page_size - 1, &hi))
+              ? UINTPTR_MAX : hi);
+      if (lo <= faulting_address && faulting_address <= hi)
         signo = 0;
 
 #   if DEBUG
       {
         char buf[1024];
-        sprintf (buf,
-                 "segv_handler fault=%p base=%p size=%lx page=%lx signo=%d\n",
-                 faulting_address, stack_base, (unsigned long) stack_size,
-                 (unsigned long) page_size, signo);
-        ignore_value (write (STDERR_FILENO, buf, strlen (buf)));
+        ignore_value (write (STDERR_FILENO, buf,
+                             sprintf (buf,
+                                      ("segv_handler code=%d fault=%p base=%p"
+                                       " size=0x%zx page=0x%zx signo=%d\n"),
+                                      info->si_code, info->si_addr, stack_base,
+                                      stack_size, page_size, signo)));
       }
-#   endif
-    }
 #  endif
+    }
 
   die (signo);
 }
@@ -303,6 +321,10 @@ c_stack_action (_GL_ASYNC_SAFE void (*action) (int))
   stack_overflow_message = _("stack overflow");
   progname = getprogname ();
 
+# if SIGINFO_WORKS
+  page_size = sysconf (_SC_PAGESIZE);
+# endif
+
   sigemptyset (&act.sa_mask);
 
 # if SIGINFO_WORKS
@@ -323,8 +345,9 @@ c_stack_action (_GL_ASYNC_SAFE void (*action) (int))
   return sigaction (SIGSEGV, &act, NULL);
 }
 
-#else /* ! ((HAVE_SIGALTSTACK && HAVE_DECL_SIGALTSTACK
-             && HAVE_STACK_OVERFLOW_HANDLING) || HAVE_LIBSIGSEGV) */
+#else /* ! (USE_LIBSIGSEGV
+            || (HAVE_SIGALTSTACK && HAVE_DECL_SIGALTSTACK
+                && HAVE_STACK_OVERFLOW_HANDLING)) */
 
 int
 c_stack_action (_GL_ASYNC_SAFE void (*action) (int)  _GL_UNUSED)
diff --git a/m4/c-stack.m4 b/m4/c-stack.m4
index c3e2bddd3..e98f80fff 100644
--- a/m4/c-stack.m4
+++ b/m4/c-stack.m4
@@ -7,7 +7,7 @@
 
 # Written by Paul Eggert.
 
-# serial 17
+# serial 18
 
 AC_DEFUN([AC_SYS_XSI_STACK_OVERFLOW_HEURISTIC],
   [
@@ -34,7 +34,7 @@ AC_DEFUN([AC_SYS_XSI_STACK_OVERFLOW_HEURISTIC],
      [Define to 1 if an invalid memory address access may yield a SIGBUS.])
 
    AC_CACHE_CHECK([for working C stack overflow detection],
-     [ac_cv_sys_stack_overflow_works],
+     [gl_cv_sys_stack_overflow_works],
      [AC_RUN_IFELSE([AC_LANG_SOURCE(
            [[
             #include <unistd.h>
@@ -121,17 +121,17 @@ AC_DEFUN([AC_SYS_XSI_STACK_OVERFLOW_HEURISTIC],
               return recurse (0);
             }
            ]])],
-        [ac_cv_sys_stack_overflow_works=yes],
-        [ac_cv_sys_stack_overflow_works=no],
+        [gl_cv_sys_stack_overflow_works=yes],
+        [gl_cv_sys_stack_overflow_works=no],
         [case "$host_os" in
                    # Guess no on native Windows.
-           mingw*) ac_cv_sys_stack_overflow_works="guessing no" ;;
-           *)      ac_cv_sys_stack_overflow_works=cross-compiling ;;
+           mingw*) gl_cv_sys_stack_overflow_works="guessing no" ;;
+           *)      gl_cv_sys_stack_overflow_works=cross-compiling ;;
          esac
         ])
      ])
 
-  if test "$ac_cv_sys_stack_overflow_works" = yes; then
+  if test "$gl_cv_sys_stack_overflow_works" = yes; then
    AC_DEFINE([HAVE_STACK_OVERFLOW_HANDLING], [1],
      [Define to 1 if extending the stack slightly past the limit causes
       a SIGSEGV which can be handled on an alternate stack established
@@ -200,14 +200,14 @@ int main ()
     fi
 
    AC_CACHE_CHECK([for precise C stack overflow detection],
-     [ac_cv_sys_xsi_stack_overflow_heuristic],
+     [gl_cv_sys_xsi_stack_overflow_heuristic],
      [dnl On Linux/sparc64 (both in 32-bit and 64-bit mode), it would be wrong
       dnl to set HAVE_XSI_STACK_OVERFLOW_HEURISTIC to 1, because the third
       dnl argument passed to the segv_handler is a 'struct sigcontext *', not
       dnl an 'ucontext_t *'.  It would lead to a failure of test-c-stack2.sh.
       case "${host_os}--${host_cpu}" in
         linux*--sparc*)
-          ac_cv_sys_xsi_stack_overflow_heuristic=no
+          gl_cv_sys_xsi_stack_overflow_heuristic=no
           ;;
         *)
           AC_RUN_IFELSE(
@@ -329,14 +329,14 @@ int main ()
                   return recurse (0);
                 }
                ]])],
-            [ac_cv_sys_xsi_stack_overflow_heuristic=yes],
-            [ac_cv_sys_xsi_stack_overflow_heuristic=no],
-            [ac_cv_sys_xsi_stack_overflow_heuristic=cross-compiling])
+            [gl_cv_sys_xsi_stack_overflow_heuristic=yes],
+            [gl_cv_sys_xsi_stack_overflow_heuristic=no],
+            [gl_cv_sys_xsi_stack_overflow_heuristic=cross-compiling])
           ;;
       esac
      ])
 
-   if test $ac_cv_sys_xsi_stack_overflow_heuristic = yes; then
+   if test "$gl_cv_sys_xsi_stack_overflow_heuristic" = yes; then
      AC_DEFINE([HAVE_XSI_STACK_OVERFLOW_HEURISTIC], [1],
        [Define to 1 if extending the stack slightly past the limit causes
         a SIGSEGV, and an alternate stack can be established with sigaltstack,
@@ -353,19 +353,16 @@ AC_DEFUN([gl_PREREQ_C_STACK],
   [AC_REQUIRE([AC_SYS_XSI_STACK_OVERFLOW_HEURISTIC])
    AC_REQUIRE([gl_LIBSIGSEGV])
 
-   # for STACK_DIRECTION
-   AC_REQUIRE([AC_FUNC_ALLOCA])
-
    AC_CHECK_FUNCS_ONCE([sigaltstack])
    AC_CHECK_DECLS([sigaltstack], , , [[#include <signal.h>]])
 
-   AC_CHECK_HEADERS_ONCE([unistd.h ucontext.h])
+   AC_CHECK_HEADERS_ONCE([ucontext.h])
 
    AC_CHECK_TYPES([stack_t], , , [#include <signal.h>])
 
    dnl c-stack does not need -lsigsegv if the system has XSI heuristics.
    if test "$gl_cv_lib_sigsegv" = yes \
-       && test $"ac_cv_sys_xsi_stack_overflow_heuristic" != yes ; then
+       && test "$gl_cv_sys_xsi_stack_overflow_heuristic" != yes; then
      AC_SUBST([LIBCSTACK], [$LIBSIGSEGV])
      AC_SUBST([LTLIBCSTACK], [$LTLIBSIGSEGV])
    fi
diff --git a/modules/c-stack b/modules/c-stack
index ca2c208e1..8c898eb9e 100644
--- a/modules/c-stack
+++ b/modules/c-stack
@@ -7,15 +7,19 @@ lib/c-stack.c
 m4/c-stack.m4
 
 Depends-on:
-gettext-h
 errno
 exitfail
+getprogname
+gettext-h
 ignore-value
-unistd
+intprops
+inttypes
+libsigsegv
 raise
 sigaction
-libsigsegv
-getprogname
+stdbool
+stddef
+unistd
 
 configure.ac:
 gl_C_STACK
-- 
2.17.1


[-- Attachment #3: 0002-c-stack-output-diagnostic-in-single-write.patch --]
[-- Type: text/x-patch, Size: 4292 bytes --]

From 649a39bbd0641c8bb48ba25e5d62d03f8f36125f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 20 Sep 2020 12:52:15 -0700
Subject: [PATCH 2/2] c-stack: output diagnostic in single 'write'

* lib/c-stack.c (die): In the typical case, use just one 'write'
syscall to output the diagnostic, as this lessens interleaving.
(die, c_stack_action): Assume C99.
* modules/c-stack (Depends-on): Add c99, mempcpy.
---
 ChangeLog       |  6 ++++++
 lib/c-stack.c   | 39 ++++++++++++++++++++++++++++++---------
 modules/c-stack |  2 ++
 3 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 087b9232f..a43c32eb8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2020-09-20  Paul Eggert  <eggert@cs.ucla.edu>
 
+	c-stack: output diagnostic in single 'write'
+	* lib/c-stack.c (die): In the typical case, use just one 'write'
+	syscall to output the diagnostic, as this lessens interleaving.
+	(die, c_stack_action): Assume C99.
+	* modules/c-stack (Depends-on): Add c99, mempcpy.
+
 	c-stack: improve checking if !libsigsegv
 	If SIGINFO_WORKS, do not treat a null pointer dereference as if it
 	were a stack overflow.  Use uintptr_t and INT_ADD_WRAPV to avoid
diff --git a/lib/c-stack.c b/lib/c-stack.c
index 3649c1bfe..742eb023e 100644
--- a/lib/c-stack.c
+++ b/lib/c-stack.c
@@ -118,7 +118,6 @@ static char const * volatile progname;
 static _GL_ASYNC_SAFE _Noreturn void
 die (int signo)
 {
-  char const *message;
 # if !SIGINFO_WORKS && !USE_LIBSIGSEGV
   /* We can't easily determine whether it is a stack overflow; so
      assume that the rest of our program is perfect (!) and that
@@ -126,11 +125,34 @@ die (int signo)
   signo = 0;
 # endif
   segv_action (signo);
-  message = signo ? program_error_message : stack_overflow_message;
-  ignore_value (write (STDERR_FILENO, progname, strlen (progname)));
-  ignore_value (write (STDERR_FILENO, ": ", 2));
-  ignore_value (write (STDERR_FILENO, message, strlen (message)));
-  ignore_value (write (STDERR_FILENO, "\n", 1));
+  char const *message = signo ? program_error_message : stack_overflow_message;
+
+  /* If the message is short, write it all at once to avoid
+     interleaving with other messages.  Avoid writev as it is not
+     documented to be async-signal-safe.  */
+  size_t prognamelen = strlen (progname);
+  size_t messagelen = strlen (message);
+  static char const separator[] = {':', ' '};
+  char buf[SIGSTKSZ / 16 + sizeof separator];
+  ptrdiff_t buflen;
+  if (prognamelen + messagelen < sizeof buf - sizeof separator)
+    {
+      char *p = mempcpy (buf, progname, prognamelen);
+      p = mempcpy (p, separator, sizeof separator);
+      p = mempcpy (p, message, messagelen);
+      *p++ = '\n';
+      buflen = p - buf;
+    }
+  else
+    {
+      ignore_value (write (STDERR_FILENO, progname, prognamelen));
+      ignore_value (write (STDERR_FILENO, separator, sizeof separator));
+      ignore_value (write (STDERR_FILENO, message, messagelen));
+      buf[0] = '\n';
+      buflen = 1;
+    }
+  ignore_value (write (STDERR_FILENO, buf, buflen));
+
   if (! signo)
     _exit (exit_failure);
   raise (signo);
@@ -299,9 +321,7 @@ segv_handler (int signo, siginfo_t *info, void *context _GL_UNUSED)
 int
 c_stack_action (_GL_ASYNC_SAFE void (*action) (int))
 {
-  int r;
   stack_t st;
-  struct sigaction act;
   st.ss_flags = 0;
 # if SIGALTSTACK_SS_REVERSED
   /* Irix mistakenly treats ss_sp as the upper bound, rather than
@@ -312,7 +332,7 @@ c_stack_action (_GL_ASYNC_SAFE void (*action) (int))
   st.ss_sp = alternate_signal_stack.buffer;
   st.ss_size = sizeof alternate_signal_stack.buffer;
 # endif
-  r = sigaltstack (&st, NULL);
+  int r = sigaltstack (&st, NULL);
   if (r != 0)
     return r;
 
@@ -325,6 +345,7 @@ c_stack_action (_GL_ASYNC_SAFE void (*action) (int))
   page_size = sysconf (_SC_PAGESIZE);
 # endif
 
+  struct sigaction act;
   sigemptyset (&act.sa_mask);
 
 # if SIGINFO_WORKS
diff --git a/modules/c-stack b/modules/c-stack
index 8c898eb9e..5ddf6e6f5 100644
--- a/modules/c-stack
+++ b/modules/c-stack
@@ -7,6 +7,7 @@ lib/c-stack.c
 m4/c-stack.m4
 
 Depends-on:
+c99
 errno
 exitfail
 getprogname
@@ -15,6 +16,7 @@ ignore-value
 intprops
 inttypes
 libsigsegv
+mempcpy
 raise
 sigaction
 stdbool
-- 
2.17.1

stack bounds

[Bruno Haible]

Hi Paul,

> While looking into this I discovered pthread_getattr_np + pthread_attr_getstack 
> which might have been nice for the GNU/Linux part of c-stack.c, except they're 
> not async-signal-safe. As I understand it, libsigsegv works around the 
> async-signal-safe problem by parsing /proc/self/maps with async-signal-safe 
> functions, which is quite a feat and is probably beyond what c-stack should do.

Yes, I agree: reading /proc/self/maps without calling malloc() is quite a bunch
of code.

> PS. I also found this circa-2015 Linux kernel bug related to PIE that looks like 
> it might be of interest to the libsigsegv developers
> 
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-1000253
> 
> This bug causes /proc/self/maps to report the wrong VMA (actually, overlapping 
> VMAs) for the stack. This could be worth a comment in the libsigsegv sources.

This is hardly a danger in practice: It applies to executables with a data
segment of size 128 MB or more. emacs' data segment size is 13 MB, and the one of
dockerd is 21 MB.

> Given the longstanding security problems with stack overflow (as witness 
> the name stackoverflow.com!) it is somewhat disturbing that there is still no 
> reliable way in GNU/Linux to answer the simple question "Where's my stack?" or 
> to detect and recover from stack overflow reliably. What's up with that?

It sounds what you want is a function that returns the stack bounds, in such a
way that every recursion step in a recursive function call the code can ask
"am I close to the stack bound? do I need to stop recursing?"

Such a thing cannot exist.

The reason is that the OS allows or disallows further growth of the stack
according to multiple criteria:
  - ulimit stack
  - ulimit memory
  - ulimit virtual memory
  - avoid collision with existing mmap()ed areas.

The algorithm that makes this decision (whether more stack can be allocated
or not) is undocumented and subject to change. And it changed in Linux, a
couple of years ago. Also, this algorithm may treat the primary thread
differently than other threads.

So, really, the only thing you can do is to do a sigaltstack() for
preparation, and wait for the signal when the OS decides to no longer allow
stack growth.

It's like when you write to a file on disk. On old operating systems like
DOS you could determine the maximum file size by looking at the free space
on the disk. This no longer works, because the decision whether a file
can grow is done by looking at
  - the free space,
  - the space occupied by the metadata for the current file,
  - quota.

Here too, your best bet is to let the OS do its decision and react when
it reports a write failure.

It would be technically possible to export the function that determines
the stack bounds (libsigsegv/src/stackvma-*). But it's pointless to do
so, because the stack bound is not the only factor considered by the OS.

Bruno

Re: stack bounds

[Paul Eggert]

On 9/22/20 5:58 PM, Bruno Haible wrote:
> It sounds what you want is a function that returns the stack bounds, in such a
> way that every recursion step in a recursive function call the code can ask
> "am I close to the stack bound? do I need to stop recursing?"

That shouldn't be necessary, except for functions where GCC can't prove that 
their activation records are small (and these exceptions should be quite rare at 
runtime).

I am thinking of some combination of gcc -fstack-check and/or 
-fstack-clash-protection and/or related ideas (not that I've looked into all the 
details). That is, I'm expecting help from the hardware, the kernel, and from 
GCC. Stack-exhaustion checking should "just work". I realize it's a nontrivial ask.

Re: stack bounds

[Bruno Haible]

Hi Paul,

you wrote on 2020-09-22:
> I am thinking of some combination of gcc -fstack-check and/or 
> -fstack-clash-protection and/or related ideas (not that I've looked into all the 
> details). That is, I'm expecting help from the hardware, the kernel, and from 
> GCC. Stack-exhaustion checking should "just work". I realize it's a nontrivial ask.

gcc -fstack-clash-protection does exactly what most people expect; as you say,
with the help from the hardware, the kernel, and GCC.
  - The hardware provides the MMU and the concept of page tables, which are
    the basis of mprotect().
  - The kernel provides mprotect; organizes a guard page at the bottom of the
    stack; and grows the stack if an access to the guard page is made.
  - 'gcc -fstack-clash-protection' arranges that a stack frame that is larger
    than one page is accessed in single-page steps, from top to bottom, so
    that the kernel will grow the stack and not report a SIGSEGV. Test code
    below.
This is how it's done on Windows.

On Linux, the kernel allows the stack to grow by any amount, if it does not
become closer than 1 MB to another VMA and does not violate the set limits.
See linux/mm/mmap.c:expand_downwards and linux/mm/mmap.c:acct_stack_growth.
Therefore on Linux, there is no need for a guard page and no need for
'gcc -fstack-clash-protection'.

In both cases, it "just works". What other features around stack bounds checking
are you thinking of?

gcc -fstack-check is unrelated: It merely adds a canary word in each stack
frame and checks that this word is still present upon return. So, it's merely
a defense against buffer overflows on the stack.

Bruno

========================= foo.c =====================
void bar (int *array);

void foo (int n)
{
  int array[n];
  for (int i = 0; i < n; i++)
    array[i] = i*i;
  bar (array);
}
=====================================================

gcc -fstack-clash-protection -O2 -S foo.c
========================= foo.s =====================
	.file	"foo.c"
	.text
	.p2align 4
	.globl	foo
	.type	foo, @function
foo:
.LFB0:
	.cfi_startproc
	pushq	%rbp
	.cfi_def_cfa_offset 16
	.cfi_offset 6, -16
	movslq	%edi, %rcx
	leaq	15(,%rcx,4), %rax
	movq	%rcx, %rdx
	movq	%rax, %rsi
	andq	$-4096, %rax
	movq	%rsp, %rdi
	movq	%rsp, %rbp
	.cfi_def_cfa_register 6
	andq	$-16, %rsi
	subq	%rax, %rdi
	cmpq	%rdi, %rsp
	je	.L3
.L13:
	subq	$4096, %rsp
	orq	$0, 4088(%rsp)
	cmpq	%rdi, %rsp
	jne	.L13
.L3:
	andl	$4095, %esi
	subq	%rsi, %rsp
	testq	%rsi, %rsi
	jne	.L14
.L4:
	movq	%rsp, %rdi
	testl	%edx, %edx
	jle	.L5
	xorl	%eax, %eax
	.p2align 4,,10
	.p2align 3
.L6:
	movl	%eax, %edx
	imull	%eax, %edx
	movl	%edx, (%rdi,%rax,4)
	addq	$1, %rax
	cmpq	%rcx, %rax
	jne	.L6
.L5:
	call	bar
	leave
	.cfi_remember_state
	.cfi_def_cfa 7, 8
	ret
	.p2align 4,,10
	.p2align 3
.L14:
	.cfi_restore_state
	orq	$0, -8(%rsp,%rsi)
	jmp	.L4
	.cfi_endproc
.LFE0:
	.size	foo, .-foo
	.ident	"GCC: (GNU) 10.2.0"
	.section	.note.GNU-stack,"",@progbits
=====================================================

Re: stack bounds

[Paul Eggert]

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

On 10/10/20 5:08 AM, Bruno Haible wrote:
> On Linux, the kernel allows the stack to grow by any amount, if it does not
> become closer than 1 MB to another VMA and does not violate the set limits.
> See linux/mm/mmap.c:expand_downwards and linux/mm/mmap.c:acct_stack_growth.
> Therefore on Linux, there is no need for a guard page and no need for
> 'gcc -fstack-clash-protection'.

There's still a need, if a function declares a large local variable, as the 
stack pointer can jump around the 1 MB barrier and trash other storage. If I 
compile the attached program with 'gcc -m32 -O2 stackish.c' on Fedora 31 x86-64, 
the program exits with status 255 (instead of crashing with a stack overflow as 
it should), because the stack has overflowed and has stomped on the heap. So 
stack overflow checking is not "just working", at least for this particular case.

[-- Attachment #2: stackish.c --]
[-- Type: text/x-csrc, Size: 447 bytes --]

#include <stdlib.h>
#include <string.h>

int
growby (int bsize, int argc)
{
  char b[bsize];
  for (int i = 0; i <= argc + 256; i++)
    b[i] = i;
  return b[argc] - b[argc + 256];
}


int
main (int argc, char **argv)
{
  int psize = argc + 1024 * 1024 * 1024;
  char *p = calloc (psize, 1);
  int bsize = (char *) &p - (p + psize/2);
  int status = growby (bsize, argc);
  for (int i = 0; i < psize; i++)
    status |= p[i]++;
  return status;
}

Re: stack bounds

[Bruno Haible]

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

Paul Eggert wrote:
> > On Linux, the kernel allows the stack to grow by any amount, if it does not
> > become closer than 1 MB to another VMA and does not violate the set limits.
> > See linux/mm/mmap.c:expand_downwards and linux/mm/mmap.c:acct_stack_growth.
> > Therefore on Linux, there is no need for a guard page and no need for
> > 'gcc -fstack-clash-protection'.
> 
> There's still a need, if a function declares a large local variable, as the 
> stack pointer can jump around the 1 MB barrier and trash other storage. If I 
> compile the attached program with 'gcc -m32 -O2 stackish.c' on Fedora 31 x86-64, 
> the program exits with status 255 (instead of crashing with a stack overflow as 
> it should), because the stack has overflowed and has stomped on the heap. So 
> stack overflow checking is not "just working", at least for this particular case.

Oh, I see: your program is not getting near the heap with the stack, it is
getting directly *into* the heap (because it fills the bottom of array 'b'
without having filled the rest of 'b' first).

gcc -fstack-clash-protection -m32 -O2 stackish.c fixes this issue.

So, you want 'gcc -fstack-clash-protection' [1] to become enabled by default?
Some distros are doing this already:
  - Ubuntu 20.04 [2] (also -fstack-clash-protection is part of the default
    gcc flags for users),
  - RHEL 8 [1] (but apparently not by default for user-compiled programs),
and the Firefox people are considering it [3].

Bruno

[1] https://developers.redhat.com/blog/2020/05/22/stack-clash-mitigation-in-gcc-part-3/
[2] https://lists.ubuntu.com/archives/ubuntu-devel/2019-June/040741.html
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1588710

[-- Attachment #2: stackish.s.ubuntu --]
[-- Type: text/plain, Size: 3300 bytes --]

	.file	"stackish.c"
	.text
	.p2align 4
	.globl	growby
	.type	growby, @function
growby:
.LFB27:
	.cfi_startproc
	endbr64
	pushq	%rbp
	.cfi_def_cfa_offset 16
	.cfi_offset 6, -16
	movslq	%edi, %rdi
	addq	$15, %rdi
	movq	%rsp, %rbp
	.cfi_def_cfa_register 6
	subq	$16, %rsp
	movq	%fs:40, %rax
	movq	%rax, -8(%rbp)
	xorl	%eax, %eax
	movq	%rsp, %rdx
	movq	%rdi, %rax
	andq	$-4096, %rdi
	subq	%rdi, %rdx
	andq	$-16, %rax
	cmpq	%rdx, %rsp
	je	.L3
.L14:
	subq	$4096, %rsp
	orq	$0, 4088(%rsp)
	cmpq	%rdx, %rsp
	jne	.L14
.L3:
	andl	$4095, %eax
	subq	%rax, %rsp
	testq	%rax, %rax
	jne	.L15
.L4:
	movl	%esi, %r8d
	movq	%rsp, %rcx
	addl	$256, %r8d
	js	.L5
	movl	%r8d, %edi
	xorl	%eax, %eax
	.p2align 4,,10
	.p2align 3
.L6:
	movq	%rax, %rdx
	movb	%al, (%rcx,%rax)
	addq	$1, %rax
	cmpq	%rdx, %rdi
	jne	.L6
.L5:
	movslq	%esi, %rsi
	movslq	%r8d, %r8
	movsbl	(%rcx,%rsi), %eax
	movsbl	(%rcx,%r8), %edx
	subl	%edx, %eax
	movq	-8(%rbp), %rsi
	xorq	%fs:40, %rsi
	jne	.L16
	leave
	.cfi_remember_state
	.cfi_def_cfa 7, 8
	ret
	.p2align 4,,10
	.p2align 3
.L15:
	.cfi_restore_state
	orq	$0, -8(%rsp,%rax)
	jmp	.L4
.L16:
	call	__stack_chk_fail@PLT
	.cfi_endproc
.LFE27:
	.size	growby, .-growby
	.section	.text.startup,"ax",@progbits
	.p2align 4
	.globl	main
	.type	main, @function
main:
.LFB28:
	.cfi_startproc
	endbr64
	pushq	%rbp
	.cfi_def_cfa_offset 16
	.cfi_offset 6, -16
	movl	$1, %esi
	movq	%rsp, %rbp
	.cfi_def_cfa_register 6
	pushq	%r12
	.cfi_offset 12, -24
	leal	1073741824(%rdi), %r12d
	pushq	%rbx
	.cfi_offset 3, -32
	movl	%edi, %ebx
	movslq	%r12d, %rdi
	subq	$16, %rsp
	movq	%fs:40, %rax
	movq	%rax, -24(%rbp)
	xorl	%eax, %eax
	call	calloc@PLT
	movl	%r12d, %edx
	movq	%rsp, %rdi
	shrl	$31, %edx
	movq	%rax, %rcx
	movq	%rax, -32(%rbp)
	leaq	-32(%rbp), %rax
	addl	%r12d, %edx
	sarl	%edx
	movslq	%edx, %rdx
	addq	%rcx, %rdx
	movq	%rsp, %rcx
	subq	%rdx, %rax
	cltq
	addq	$15, %rax
	movq	%rax, %rdx
	andq	$-4096, %rax
	subq	%rax, %rcx
	andq	$-16, %rdx
	movq	%rcx, %rax
	cmpq	%rax, %rsp
	je	.L19
.L32:
	subq	$4096, %rsp
	orq	$0, 4088(%rsp)
	cmpq	%rax, %rsp
	jne	.L32
.L19:
	andl	$4095, %edx
	subq	%rdx, %rsp
	testq	%rdx, %rdx
	jne	.L33
.L20:
	movl	%ebx, %r8d
	movq	%rsp, %rcx
	addl	$256, %r8d
	js	.L21
	movl	%r8d, %esi
	xorl	%eax, %eax
	.p2align 4,,10
	.p2align 3
.L22:
	movq	%rax, %rdx
	movb	%al, (%rcx,%rax)
	addq	$1, %rax
	cmpq	%rdx, %rsi
	jne	.L22
.L21:
	movslq	%ebx, %rax
	movslq	%r8d, %r8
	movsbl	(%rcx,%rax), %eax
	movsbl	(%rcx,%r8), %edx
	movq	%rdi, %rsp
	subl	%edx, %eax
	testl	%r12d, %r12d
	jle	.L17
	leal	1073741823(%rbx), %r8d
	xorl	%ecx, %ecx
	.p2align 4,,10
	.p2align 3
.L24:
	movq	-32(%rbp), %rsi
	addq	%rcx, %rsi
	movsbl	(%rsi), %edx
	leal	1(%rdx), %edi
	orl	%edx, %eax
	movq	%rcx, %rdx
	addq	$1, %rcx
	movb	%dil, (%rsi)
	cmpq	%rdx, %r8
	jne	.L24
.L17:
	movq	-24(%rbp), %rbx
	xorq	%fs:40, %rbx
	jne	.L34
	leaq	-16(%rbp), %rsp
	popq	%rbx
	popq	%r12
	popq	%rbp
	.cfi_remember_state
	.cfi_def_cfa 7, 8
	ret
.L33:
	.cfi_restore_state
	orq	$0, -8(%rsp,%rdx)
	jmp	.L20
.L34:
	call	__stack_chk_fail@PLT
	.cfi_endproc
.LFE28:
	.size	main, .-main
	.ident	"GCC: (Ubuntu 9.3.0-10ubuntu2) 9.3.0"
	.section	.note.GNU-stack,"",@progbits
	.section	.note.gnu.property,"a"
	.align 8
	.long	 1f - 0f
	.long	 4f - 1f
	.long	 5
0:
	.string	 "GNU"
1:
	.align 8
	.long	 0xc0000002
	.long	 3f - 2f
2:
	.long	 0x3
3:
	.align 8
4:

Re: stack bounds

[Paul Eggert]

On 10/10/20 2:49 PM, Bruno Haible wrote:
> gcc -fstack-clash-protection -m32 -O2 stackish.c fixes this issue.

Yes. However, the GCC manual says this about -fstack-clash-protection:

      Most targets do not fully support stack clash protection.  However,
      on those targets '-fstack-clash-protection' will protect dynamic
      stack allocations.  '-fstack-clash-protection' may also provide
      limited protection for static stack allocations if the target
      supports '-fstack-check=specific'.

which is not as close to "it should just work" as I'd like, especially when I go 
read the section on -fstack-check. I suppose I need to look at the output of gcc 
-S -fstack-clash-protection on my platform (and understand what the OS does) to 
know whether stack overflow is detected reliably.

That being said, it does look like a reliability win if we start using 
-fstack-clash-protection on platforms like Fedora x86-64 that support it and do 
not enable it by default. Perhaps we should have a Gnulib or Autoconf macro that 
does that.

Re: stack bounds

[Bruno Haible]

Paul Eggert wrote:
> That being said, it does look like a reliability win if we start using 
> -fstack-clash-protection on platforms like Fedora x86-64 that support it and do 
> not enable it by default. Perhaps we should have a Gnulib or Autoconf macro that 
> does that.

Yes, such a macro would be useful. To be used at the discretion of the package
maintainer (e.g. through a Gnulib module, similar to the 'largefile' module).

But do you have an overview which targets are meant in the doc?

  "old-style stack checking is also the fallback method for ‘specific’ if no
   target support has been added in the compiler."

  "Most targets do not fully support stack clash protection."

Bruno

Re: stack bounds

[Paul Eggert]

On 10/11/20 3:08 PM, Bruno Haible wrote:
> But do you have an overview which targets are meant in the doc?

Unfortunately not. I expect it'd have to be determined from the comments in GCC, 
and it might also need info from various OSes and/or linkers.