[PATCH] main.c: obviously initialize smark

[PATCH] main.c: obviously initialize smark

[Fabian Rast]

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

Otherwise an optimizing compiler (with LTO) might conclude that an
undefined value is used in the popstackmark call in the setjmp handler,
causing undefined behavior.
---
 src/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/main.c b/src/main.c
index 5d25b8d..1ab8c04 100644
--- a/src/main.c
+++ b/src/main.c
@@ -105,6 +105,7 @@ main(int argc, char **argv)
 #endif
 
 	setlocale(LC_ALL, "");
+	setstackmark(&smark);
 
 	state = 0;
 	if (unlikely(setjmp(main_handler.loc))) {
@@ -149,7 +150,6 @@ main(int argc, char **argv)
 #endif
 	mypid = rootpid = getpid();
 	init();
-	setstackmark(&smark);
 	login = procargs(argv);
 	if (login) {
 		state = 1;
-- 
2.52.0

Hello,

I discovered this bug in a full LTO build of dash, where
triggering a longjmp using Ctrl-C caused a crash.

Reproduction, depends on compiler...

tmp $ clang --version
clang version 21.1.8 (AerynOS)
Target: x86_64-aerynos-linux
Thread model: posix
InstalledDir: /usr/bin
tmp $ git clone https://git.kernel.org/pub/scm/utils/dash/dash.git
tmp 0$ cd dash
dash $ ./autogen.sh
dash $ CC=clang CFLAGS='-O2 -flto=full' LDFLAGS=-flto=full ./configure --enable-glob --with-libedit
dash $ make
dash $ ./src/dash
$ sleep 10
^C
free(): invalid pointer
The process was killed by SIGABRT: Aborted

As far as I understand, what is happening is that in the full LTO
configuration, clang deduces that if the setjmp condition is true,
smark is still undefined, causing undefined behavior.

Of course, due to how setjmp works, the condition is _never_ true before
smark is initialized, but clang does not realize this.

I figured the optimal solution would be to move the initialization
before the setjmp if statement.
As far as I can see this should not cause other problems, although
moving the call before the `init()` feels weird
- i would love your opinon on this!

The other option would be to just zero initialize smark, but this
would be redundant in all practical executions.

Best Regards,
Fabian Rast

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] main.c: obviously initialize smark

[Herbert Xu]

Fabian Rast <fabian.rast@tum.de> wrote:
> [-- text/plain, encoding quoted-printable, charset: UTF-8, 73 lines --]
> 
> Otherwise an optimizing compiler (with LTO) might conclude that an
> undefined value is used in the popstackmark call in the setjmp handler,
> causing undefined behavior.
> ---
> src/main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Thanks for the patch.  But I think this is really a compiler bug.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt