From 8b07d2a011f7dfe173e869d12ada4dd51e836cde Mon Sep 17 00:00:00 2001
From: Martijn Dekker <martijn@inlv.org>
Date: Fri, 5 Jun 2020 16:03:07 +0200
Subject: [PATCH] Fix various crashes by removing invalid memccpy() use

The sfputr() function (put out a null-terminated string) contained
a use of memccpy() that was invalid and could cause crashes,
because the buffer it was copying into could overlap or even be
identical with the buffer being copied from.

Among (probably) other things, this commit fixes a crash in 'print
-v' (print a compound variable structure) on macOS, that caused the
comvar.sh and comvario.sh regression tests to fail spectacularly.
Now they pass.

Issue discovered and fixed by Kurtis Rader in the abandoned ksh2020
branch; this commit backports the fix. He wrote:

| #if _lib_memccpy && !__ia64 /* these guys may never get it right */
|
| The problem is that assertion is wrong. It implies that the libc
| implementation of memccpy() on IA64 is broken. Which is
| incorrect. The problem is the AST sfputr() function is depending
| on what is explicitly undefined behavior in the face of
| overlapping source and destination buffers.
| [...] Using memccpy() simply complicates the code and is unlikely
| to be measurably, let alone noticeably, faster.

Further discussion/analysis: https://github.com/att/ast/issues/78

src/lib/libast/sfio/sfputr.c:
- Remove memccpy use. Always use the manual copying loop.

(cherry picked from commit fbe3c83335256cb714a4aa21f555083c9f1d71d8)
---
 NEWS                         |  4 ++++
 src/lib/libast/sfio/sfputr.c | 13 ++++++-------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/NEWS b/NEWS
index 81c4cbcdf..57a22fc8f 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,10 @@ Any uppercase BUG_* names are modernish shell bug IDs.
     (unset PATH; PATH=/dev/null; ls); : wrongly ran 'ls'
     (unset LC_ALL; LC_ALL=badlocale); : failed to print a diagnostic
 
+- Fix crashes on some systems, including at least a crash in 'print -v' on
+  macOS, by eliminating an invalid/undefined use of memccpy() on overlapping
+  buffers in the commonly used sfputr() function.
+
 2020-06-04:
 
 - Fix BUG_KBGPID: the $! special parameter was not set if a background job
diff --git a/src/lib/libast/sfio/sfputr.c b/src/lib/libast/sfio/sfputr.c
index 21bfb0aeb..0d0f65124 100644
--- a/src/lib/libast/sfio/sfputr.c
+++ b/src/lib/libast/sfio/sfputr.c
@@ -105,16 +105,15 @@ int		rc;	/* record separator.	*/
 			break;
 		}
 
-#if _lib_memccpy && !__ia64 /* these guys may never get it right */
-		if((ps = (uchar*)memccpy(ps,s,'\0',p)) != NIL(uchar*))
-			ps -= 1;
-		else	ps  = f->next+p;
-		s += ps - f->next;
-#else
+		/*
+		 * Do not replace the following loop with memccpy(). The
+		 * 'ps' and 's' buffers may overlap or even point to the
+		 * same buffer. See: https://github.com/att/ast/issues/78
+		 */
 		for(; p > 0; --p, ++ps, ++s)
 			if((*ps = *s) == 0)
 				break;
-#endif
+
 		w += ps - f->next;
 		f->next = ps;
 	}