From 37459bc6c17a69defc0d6d1f568dcd0261c78511 Mon Sep 17 00:00:00 2001
From: Colin Snover
Date: Sun, 24 Sep 2017 13:10:45 +0800
Subject: SCI: Fix UB in SegManager memcpy/strcpy operations

Passing overlapping buffers to C standard library memcpy, strcpy,
and strncpy is undefined behavior. In SSCI these operations would
perform a forward copy, and most stdlib implementations do the
same, but at least newer Linux glibc on x86 copies bytes in
reverse, so just using the standard library on this platform
results in broken output.

Because SSCI used a blind forward copy instead of memmove for
overlapping copy operations, this patch implements an explicit
forward copy to ensure that overlapping copies continue to operate
the same as in SSCI.

This fixes the Island of Dr. Brain v1.1 flamingo puzzle
(script 185, flamingos::init, localCall 4c3) on platforms that do
not perform forward copy in memcpy/strcpy/strncpy.

Thanks to @moralrecordings for research on this bug and an initial
patch using memmove.

Closes gh-1034.
---
 engines/sci/engine/seg_manager.cpp | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

(limited to 'engines')

diff --git a/engines/sci/engine/seg_manager.cpp b/engines/sci/engine/seg_manager.cpp
index aa70d5f838..45666336e9 100644
--- a/engines/sci/engine/seg_manager.cpp
+++ b/engines/sci/engine/seg_manager.cpp
@@ -614,7 +614,25 @@ static inline void setChar(const SegmentRef &ref, uint offset, byte value) {
 		val->setOffset((val->getOffset() & 0xff00) | value);
 }
 
-// TODO: memcpy, strcpy and strncpy could maybe be folded into a single function
+template <bool STRING>
+static void forwardCopy(byte *dest, const byte *src, size_t n) {
+	const bool zeroPad = (STRING && n != 0xFFFFFFFFU);
+
+	while (n) {
+		--n;
+		const byte b = *src++;
+		*dest++ = b;
+		if (STRING && b == '\0') {
+			break;
+		}
+	}
+	if (zeroPad) {
+		while (n--) {
+			*dest++ = '\0';
+		}
+	}
+}
+
 void SegManager::strncpy(reg_t dest, const char* src, size_t n) {
 	SegmentRef dest_r = dereference(dest);
 	if (!dest_r.isValid()) {
@@ -624,11 +642,7 @@ void SegManager::strncpy(reg_t dest, const char* src, size_t n) {
 
 
 	if (dest_r.isRaw) {
-		// raw -> raw
-		if (n == 0xFFFFFFFFU)
-			::strcpy((char *)dest_r.raw, src);
-		else
-			::strncpy((char *)dest_r.raw, src, n);
+		forwardCopy<true>(dest_r.raw, (const byte *)src, n);
 	} else {
 		// raw -> non-raw
 		for (uint i = 0; i < n; i++) {
@@ -711,7 +725,7 @@ void SegManager::memcpy(reg_t dest, const byte* src, size_t n) {
 
 	if (dest_r.isRaw) {
 		// raw -> raw
-		::memcpy((char *)dest_r.raw, src, n);
+		forwardCopy<false>(dest_r.raw, src, n);
 	} else {
 		// raw -> non-raw
 		for (uint i = 0; i < n; i++)
@@ -767,7 +781,7 @@ void SegManager::memcpy(byte *dest, reg_t src, size_t n) {
 
 	if (src_r.isRaw) {
 		// raw -> raw
-		::memcpy(dest, src_r.raw, n);
+		forwardCopy<false>(dest, src_r.raw, n);
 	} else {
 		// non-raw -> raw
 		for (uint i = 0; i < n; i++) {
-- 
cgit v1.2.3