valter/dosbox-staging
1
0
Fork 0
Code Releases Activity

Fix Coverity warnings about BlockRead/Write

Browse source 
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`.
This commit is contained in:
Patryk Obara 2020-04-06 11:51:05 +02:00 • committed by Patryk Obara
parent a6792a44ad
commit c7287e116e
1 changed files with 21 additions and 14 deletions
Download patch file Download diff file Expand all files Collapse all files

35
src/ints/ems.cpp
View file

@ -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) {