From f24040ee454baabbc7942ba6e0adc5ed99d27363 Mon Sep 17 00:00:00 2001
From: Martijn Dekker <martijn@inlv.org>
Date: Fri, 19 Aug 2022 15:25:40 +0100
Subject: [PATCH] Fix use after free in sh_funstaks() (re: 69d37d5e)

The referenced commit introduced the NIL (NULL) assignment in:

	stakdelete(slpold->slptr);
	slpold->slptr = NIL(Stak_t*);

First the stack is closed/freed with stakdelete() a.k.a.
stkclose(), then its pointer is reset. Looks correct, right?

Wrong: slpold may itself be in the allocated region that
slpold->slptr points to. That's because we're dealing with a linked
list of stacks, in which a pointer on each stack points to the next
stack. So there are scenarios in which, after the stakdelete()
call, dereferencing slpold is a use after free.

Most systems quietly tolerate this use after free. But, according
to @JohnoKing's testing, this bug was causing 23 crashes in the
regression tests after compiling ksh with AddressSanitizer enabled.

src/cmd/ksh93/sh/parse.c: sh_funstaks():
- Save the value of slpold->slptr and reset that pointer before
  calling stakdelete() a.k.a. stkclose().

Resolves: https://github.com/ksh93/ksh/issues/517
---
 src/cmd/ksh93/include/argnod.h  |  2 +-
 src/cmd/ksh93/include/version.h |  2 +-
 src/cmd/ksh93/sh/parse.c        | 10 +++++++++-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/cmd/ksh93/include/argnod.h b/src/cmd/ksh93/include/argnod.h
index 6c160688..636e473b 100644
--- a/src/cmd/ksh93/include/argnod.h
+++ b/src/cmd/ksh93/include/argnod.h
@@ -53,7 +53,7 @@ struct comnod
 #define COMSCAN		(01<<COMBITS)
 #define COMFIXED	(02<<COMBITS)
 
-struct slnod 	/* struct for link list of stacks */
+struct slnod 	/* struct for linked list of stacks */
 {
 	struct slnod	*slnext;
 	struct slnod	*slchild;
diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h
index 48c67899..d102799c 100644
--- a/src/cmd/ksh93/include/version.h
+++ b/src/cmd/ksh93/include/version.h
@@ -18,7 +18,7 @@
 
 #define SH_RELEASE_FORK	"93u+m"		/* only change if you develop a new ksh93 fork */
 #define SH_RELEASE_SVER	"1.0.3-alpha"	/* semantic version number: https://semver.org */
-#define SH_RELEASE_DATE	"2022-08-18"	/* must be in this format for $((.sh.version)) */
+#define SH_RELEASE_DATE	"2022-08-19"	/* must be in this format for $((.sh.version)) */
 #define SH_RELEASE_CPYR	"(c) 2020-2022 Contributors to ksh " SH_RELEASE_FORK
 
 /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */
diff --git a/src/cmd/ksh93/sh/parse.c b/src/cmd/ksh93/sh/parse.c
index 3d545170..070bd2df 100644
--- a/src/cmd/ksh93/sh/parse.c
+++ b/src/cmd/ksh93/sh/parse.c
@@ -528,8 +528,16 @@ void sh_funstaks(register struct slnod *slp,int flag)
 		{
 			if(flag<=0)
 			{
-				stakdelete(slpold->slptr);
+				/*
+				 * Since we're dealing with a linked list of stacks, slpold may be inside the allocated region
+				 * pointed to by slpold->slptr, meaning the stakdelete() call may invalidate slpold as well as
+				 * slpold->slptr. So if we do 'stakdelete(slpold->slptr); slpold->slptr = NIL(Stak_t*)' as may
+				 * seem obvious, the assignment may be a use-after-free of slpold. Therefore, save the pointer
+				 * value and reset the pointer before closing/freeing the stack.
+				 */
+				Stak_t *sp = slpold->slptr;
 				slpold->slptr = NIL(Stak_t*);
+				stakdelete(sp);
 			}
 			else
 				staklink(slpold->slptr);