From c7287e116e54518d52ac5ea01d36608a6fd2f64a Mon Sep 17 00:00:00 2001
From: Patryk Obara <dreamer.tan@gmail.com>
Date: Mon, 6 Apr 2020 11:51:05 +0200
Subject: [PATCH] Fix Coverity warnings about BlockRead/Write
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

These are false-positive findings, but they touch flimsy part of code,
that could very easily break.

In all 4 cases the code looked like this:

    uint8_t buf[MEM_PAGE_SIZE];  // old lines 600, 601
    …
    if (region.bytes > MEM_PAGE_SIZE)
            toread = MEM_PAGE_SIZE; // old line 635
    else
            toread = region.bytes;

    // assert toread <= MEM_PAGE_SIZE
    if (toread < remain) {
            MEM_BlockWrite(…, buf, toread);
    } else {
            MEM_BlockWrite(…, buf, remain);
            MEM_BlockWrite(…, &buf[remain], toread - remain);
            //                ~~~~~~~~~~~~
            //                ^
            //                Coverity flags buffer overrun here
            //                when: toread == remain == MEM_PAGE_SIZE
            //                because buf has size MEM_PAGE_SIZE
            //
            // Sometimes it's MEM_BlockRead, but the problem is the same
    }

In such cases, second MEM_BlockWrite is a no-op because
(toread - remain == 0), but Coverity did not reach this point in
analysis (Coverity is right to check the corner case, but didn't know,
that we can assert toread <= MEM_PAGE_SIZE, so the corner case is the
only value triggering this buffer overrun).

Change `if (toread < remain)` to `if (toread <= remain)`, so corner case
will never trigger second (no-op) MEM_BlockRead/Write inside `else`.
---
 src/ints/ems.cpp | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/src/ints/ems.cpp b/src/ints/ems.cpp
index c346b408..141316e2 100644
--- a/src/ints/ems.cpp
+++ b/src/ints/ems.cpp
@@ -16,9 +16,10 @@
  *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
  */
 
+#include <algorithm>
+#include <cstring>
+#include <cstdlib>
 
-#include <string.h>
-#include <stdlib.h>
 #include "dosbox.h"
 #include "callback.h"
 #include "mem.h"
@@ -596,10 +597,10 @@ static void LoadMoveRegion(PhysPt data,MoveRegion & region) {
 	region.dest_page_seg=mem_readw(data+0x10);
 }
 
-static Bit8u MemoryRegion(void) {
+static uint8_t MemoryRegion()
+{
 	MoveRegion region;
-	Bit8u buf_src[MEM_PAGE_SIZE];
-	Bit8u buf_dest[MEM_PAGE_SIZE];
+
 	if (reg_al>1) {
 		LOG(LOG_MISC,LOG_ERROR)("EMS:Call %2X Subfunction %2X not supported",reg_ah,reg_al);
 		return EMM_FUNC_NOSUP;
@@ -631,15 +632,19 @@ static Bit8u MemoryRegion(void) {
 		dest_off=region.dest_offset&(MEM_PAGE_SIZE-1);
 		dest_remain=MEM_PAGE_SIZE-dest_off;
 	}
-	Bitu toread;
-	while (region.bytes>0) {
-		if (region.bytes>MEM_PAGE_SIZE) toread=MEM_PAGE_SIZE;
-		else toread=region.bytes;
+
+	uint8_t buf_src[MEM_PAGE_SIZE];
+	uint8_t buf_dest[MEM_PAGE_SIZE];
+
+	while (region.bytes > 0) {
+		const size_t toread = std::min<size_t>(region.bytes, MEM_PAGE_SIZE);
+
 		/* Read from the source */
 		if (!region.src_type) {
 			MEM_BlockRead(src_mem,buf_src,toread);
 		} else {
-			if (toread<src_remain) {
+			assert(toread <= MEM_PAGE_SIZE);
+			if (toread <= src_remain) {
 				MEM_BlockRead((src_handle*MEM_PAGE_SIZE)+src_off,buf_src,toread);
 			} else {
 				MEM_BlockRead((src_handle*MEM_PAGE_SIZE)+src_off,buf_src,src_remain);
@@ -652,7 +657,8 @@ static Bit8u MemoryRegion(void) {
 			if (!region.dest_type) {
 				MEM_BlockRead(dest_mem,buf_dest,toread);
 			} else {
-				if (toread<dest_remain) {
+				assert(toread <= MEM_PAGE_SIZE);
+				if (toread <= dest_remain) {
 					MEM_BlockRead((dest_handle*MEM_PAGE_SIZE)+dest_off,buf_dest,toread);
 				} else {
 					MEM_BlockRead((dest_handle*MEM_PAGE_SIZE)+dest_off,buf_dest,dest_remain);
@@ -663,7 +669,8 @@ static Bit8u MemoryRegion(void) {
 			if (!region.src_type) {
 				MEM_BlockWrite(src_mem,buf_dest,toread);
 			} else {
-				if (toread<src_remain) {
+				assert(toread <= MEM_PAGE_SIZE);
+				if (toread <= src_remain) {
 					MEM_BlockWrite((src_handle*MEM_PAGE_SIZE)+src_off,buf_dest,toread);
 				} else {
 					MEM_BlockWrite((src_handle*MEM_PAGE_SIZE)+src_off,buf_dest,src_remain);
@@ -675,7 +682,8 @@ static Bit8u MemoryRegion(void) {
 		if (!region.dest_type) {
 			MEM_BlockWrite(dest_mem,buf_src,toread);
 		} else {
-			if (toread<dest_remain) {
+			assert(toread <= MEM_PAGE_SIZE);
+			if (toread <= dest_remain) {
 				MEM_BlockWrite((dest_handle*MEM_PAGE_SIZE)+dest_off,buf_src,toread);
 			} else {
 				MEM_BlockWrite((dest_handle*MEM_PAGE_SIZE)+dest_off,buf_src,dest_remain);
@@ -692,7 +700,6 @@ static Bit8u MemoryRegion(void) {
 	return EMM_NO_ERROR;
 }
 
-
 static Bitu INT67_Handler(void) {
 	Bitu i;
 	switch (reg_ah) {