[Scummvm-git-logs] scummvm master -> 37459bc6c17a69defc0d6d1f568dcd0261c78511

csnover csnover at users.noreply.github.com
Sun Oct 29 19:19:30 CET 2017


This automated email contains information about 1 new commit which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .

Summary:
37459bc6c1 SCI: Fix UB in SegManager memcpy/strcpy operations


Commit: 37459bc6c17a69defc0d6d1f568dcd0261c78511
    https://github.com/scummvm/scummvm/commit/37459bc6c17a69defc0d6d1f568dcd0261c78511
Author: Colin Snover (github.com at zetafleet.com)
Date: 2017-10-29T13:18:37-05:00

Commit Message:
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.

Changed paths:
    engines/sci/engine/seg_manager.cpp


diff --git a/engines/sci/engine/seg_manager.cpp b/engines/sci/engine/seg_manager.cpp
index aa70d5f..4566633 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++) {





More information about the Scummvm-git-logs mailing list