Minor fixes

src/BufferCopy/RDRAMtoColorBuffer.cpp

@@ -147,7 +147,7 @@ bool _copyPixelsFromRdram(u32 _address, const std::vector<u32> & _vecAddress, u3
 		idx = (_vecAddress[i] - _address) / szPixel;
 		w = idx % _width;
 		h = idx / _width;
-		if (h > _height)
+		if (h >= _height)
 			return false;
 		col = src[idx];
 		summ += col;

src/Graphics/OpenGLContext/GLSL/glsl_CombinerProgramUniformFactoryCommon.cpp

@@ -758,15 +758,17 @@ class ULights : public UniformGroup
 
 	void update(bool _force) override
 	{
-		for (u32 i = 0; i <= gSP.numLights; ++i) {
+		const u32 numLights = std::min(gSP.numLights, _numLights);
+		for (u32 i = 0; i <= numLights; ++i) {
 			uLightDirection[i].set(gSP.lights.xyz[i], _force);
 			uLightColor[i].set(gSP.lights.rgb[i], _force);
 		}
 	}
 
 private:
-	fv3Uniform uLightDirection[8];
-	fv3Uniform uLightColor[8];
+	static constexpr u32 _numLights = 8;
+	fv3Uniform uLightDirection[_numLights];
+	fv3Uniform uLightColor[_numLights];
 };
 
 } //nameless namespace

src/Graphics/OpenGLContext/GLSL/glsl_ShaderStorage.cpp

@@ -228,11 +228,12 @@ CombinerProgramImpl * _readCombinerProgramFromStream(std::istream & _is,
 	std::vector<char> binary(binaryLength);
 	_is.read(binary.data(), binaryLength);
 
-	GLuint program = glCreateProgram();
+	const GLuint program = glCreateProgram();
 	const bool isRect = _cmbKey.isRectKey();
 	glsl::Utils::locateAttributes(program, isRect, cmbInputs.usesTexture());
 	glProgramBinary(program, binaryFormat, binary.data(), binaryLength);
 	if (!glsl::Utils::checkProgramLinkStatus(program, true)) {
+		glDeleteProgram(program);
 		return nullptr;
 	}
 

src/Graphics/OpenGLContext/ThreadedOpenGl/opengl_ObjectPool.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <atomic>
 #include <memory>
 #include <vector>
 
@@ -23,7 +24,7 @@ namespace opengl {
 		void setObjectId(int _objectId);
 	private:
 
-		bool m_inUse;
+		std::atomic<bool> m_inUse;
 		int m_poolId;
 		int m_objectId;
 	};

src/Graphics/OpenGLContext/ThreadedOpenGl/opengl_WrappedFunctions.h

@@ -3382,8 +3382,10 @@ class GlMapBufferRangeWriteAsyncCommand : public OpenGlCommand
 
 	void commandToExecute() override
 	{
-		const char* data = OpenGlCommand::m_ringBufferPool.getBufferFromPool(m_data);
 		void* buffer_pointer = ptrMapBufferRange(m_target, m_offset, m_length, m_access);
+		if (buffer_pointer == nullptr)
+			return;
+		const char* data = OpenGlCommand::m_ringBufferPool.getBufferFromPool(m_data);
 		std::copy_n(data, m_length, reinterpret_cast<char*>(buffer_pointer));
 		OpenGlCommand::m_ringBufferPool.removeBufferFromPool(m_data);
 	}
@@ -3474,14 +3476,14 @@ class GlMapBufferRangeReadAsyncCommand : public OpenGlCommand
 	void commandToExecute() override
 	{
 		void* buffer_pointer = ptrMapBufferRange(m_target, m_offset, m_length, m_access);
+		if (buffer_pointer == nullptr)
+			return;
 
-		if (buffer_pointer != nullptr) {
-			std::unique_lock<std::mutex> lock(m_mapMutex);
-			GLuint buffer = GlBindBufferCommand::getBoundBufferRender(m_target);
-			verifyBuffer(buffer, m_length);
-			auto data = m_data[buffer];
-			memcpy(data->data(), buffer_pointer, m_length);
-		}
+		std::unique_lock<std::mutex> lock(m_mapMutex);
+		GLuint buffer = GlBindBufferCommand::getBoundBufferRender(m_target);
+		verifyBuffer(buffer, m_length);
+		auto data = m_data[buffer];
+		memcpy(data->data(), buffer_pointer, m_length);
 	}
 
 	static std::shared_ptr<std::vector<u8>> getData(GLuint buffer, GLsizeiptr length)

src/Graphics/OpenGLContext/mupen64plus/mupen64plus_DisplayWindow.cpp

@@ -252,6 +252,12 @@ void DisplayWindowMupen64plus::_readScreen2(void * _dest, int * _width, int * _h
 	if (_dest == nullptr)
 		return;
 
+	// GL_PACK_ALIGNMENT defaults to 4, so with the 3 byte format below the
+	// driver pads every row up to a 4 byte boundary and writes past the
+	// width*height*3 buffer whenever width*3 is not a multiple of 4. _dest is
+	// owned by the emulator core, so the overrun lands in its memory.
+	glPixelStorei(GL_PACK_ALIGNMENT, 1);
+
 #if !defined(OS_ANDROID) && !defined(OS_IOS)
 	GLint oldMode;
 	glGetIntegerv(GL_READ_BUFFER, &oldMode);

src/Graphics/OpenGLContext/opengl_UnbufferedDrawer.cpp

@@ -119,7 +119,7 @@ void UnbufferedDrawer::drawTriangles(const graphics::Context::DrawTriangleParame
 
 	for (GLint i = 0; i < GLint(_params.elementsCount); i += 3) {
 		glMemoryBarrier(GL_SHADER_IMAGE_ACCESS_BARRIER_BIT);
-		glDrawElements(GLenum(_params.mode), 3, GL_UNSIGNED_BYTE, (u8*)_params.elements + i);
+		glDrawElements(GLenum(_params.mode), 3, GL_UNSIGNED_SHORT, (u16*)_params.elements + i);
 	}
 }
 

src/Graphics/OpenGLContext/windows/windows_DisplayWindow.cpp

@@ -73,6 +73,12 @@ void DisplayWindowWindows::_saveScreenshot()
 	gfxContext.bindFramebuffer(graphics::bufferTarget::READ_FRAMEBUFFER, graphics::ObjectHandle::defaultFramebuffer);
 	glReadBuffer(GL_FRONT);
 	pixelData = (unsigned char*)malloc(m_screenWidth * m_screenHeight * 3);
+	if (pixelData == nullptr)
+		return;
+	// GL_PACK_ALIGNMENT defaults to 4, so with a 3 byte format the driver pads
+	// every row up to a 4 byte boundary and writes past a width*height*3
+	// allocation whenever width*3 is not a multiple of 4.
+	glPixelStorei(GL_PACK_ALIGNMENT, 1);
 	glReadPixels(0, m_heightOffset, m_screenWidth, m_screenHeight, GL_RGB, GL_UNSIGNED_BYTE, pixelData);
 	if (graphics::BufferAttachmentParam(oldMode) == graphics::bufferAttachment::COLOR_ATTACHMENT0) {
 		FrameBuffer * pBuffer = frameBufferList().getCurrent();
@@ -91,6 +97,9 @@ void DisplayWindowWindows::_saveBufferContent(graphics::ObjectHandle _fbo, Cache
 	glGetIntegerv(GL_READ_BUFFER, &oldMode);
 	gfxContext.bindFramebuffer(graphics::bufferTarget::READ_FRAMEBUFFER, _fbo);
 	pixelData = (unsigned char*)malloc(_pTexture->width * _pTexture->height * 3);
+	if (pixelData == nullptr)
+		return;
+	glPixelStorei(GL_PACK_ALIGNMENT, 1);	// see _saveScreenshot
 	glReadPixels(0, 0, _pTexture->width, _pTexture->height, GL_RGB, GL_UNSIGNED_BYTE, pixelData);
 	if (graphics::BufferAttachmentParam(oldMode) == graphics::bufferAttachment::COLOR_ATTACHMENT0) {
 		FrameBuffer * pCurrentBuffer = frameBufferList().getCurrent();
@@ -268,6 +277,8 @@ void DisplayWindowWindows::_readScreen(void **_pDest, long *_pWidth, long *_pHei
 	if (*_pDest == nullptr)
 		return;
 
+	glPixelStorei(GL_PACK_ALIGNMENT, 1);	// see _saveScreenshot
+
 #ifndef GLESX
 	GLint oldMode;
 	glGetIntegerv(GL_READ_BUFFER, &oldMode);

src/N64.h

@@ -54,5 +54,15 @@ inline bool isRDRAMRangeValid(u32 _address, u32 _size)
 	return _address <= RDRAMSize && (RDRAMSize + 1 - _address) >= _size;
 }
 
+// SP DMEM is 4 KB. Unlike RDRAMSize this is a true size, not a last-valid
+// index. Several microcodes derive DMEM offsets from display list bit fields,
+// so the same care applies: written to avoid wrapping either operation.
+static const u32 DMEMSize = 0x1000;
+
+inline bool isDMEMRangeValid(u32 _address, u32 _size)
+{
+	return _address <= DMEMSize && (DMEMSize - _address) >= _size;
+}
+
 #endif
 

src/gDP.cpp

@@ -699,7 +699,7 @@ void gDPLoadBlock(u32 tile, u32 uls, u32 ult, u32 lrs, u32 dxt)
 	if (gDP.loadTile->size == G_IM_SIZ_32b)
 		gDPLoadBlock32(gDP.loadTile->uls, gDP.loadTile->lrs, dxt);
 	else if (gDP.loadTile->format == G_IM_FMT_YUV)
-		memcpy(TMEM, &RDRAM[address], bytes); // HACK!
+		memcpy(TMEM, &RDRAM[address], bytes & 0xFFF); // HACK!
 	else {
 		u32 tmemAddr = gDP.loadTile->tmem;
 		UnswapCopyWrap(RDRAM, address, reinterpret_cast<u8*>(TMEM), tmemAddr << 3, 0xFFF, bytes);

src/gSP.cpp

@@ -2363,7 +2363,10 @@ void gSPLine3D(u32 v0, u32 v1, s32 wd, u32 flag )
 
 void gSPSetStatus(u32 sid, u32 val)
 {
-	assert(sid <= 12);
+	if (sid > 12) {
+		DebugMsg(DEBUG_NORMAL | DEBUG_ERROR, "// gSPSetStatus: invalid sid %u\n", sid);
+		return;
+	}
 	gSP.status[sid>>2] = val;
 
 	DebugMsg(DEBUG_NORMAL, "gSPSetStatus sid=%u val=%u\n", sid, val);

src/uCodes/S2DEX.cpp

@@ -397,8 +397,8 @@ struct ObjCoordinates
 		const u16 objSpriteScaleW = std::max(_pObjSprite->scaleW, u16(1));
 		const u16 objSpriteScaleH = std::max(_pObjSprite->scaleH, u16(1));
 		if (_useMatrix) {
-			const u32 scaleW = (u32(objMtx.BaseScaleX) * 0x40 * objSpriteScaleW) >> 16;
-			const u32 scaleH = (u32(objMtx.BaseScaleY) * 0x40 * objSpriteScaleH) >> 16;
+			const u32 scaleW = std::max((u32(objMtx.BaseScaleX) * 0x40 * objSpriteScaleW) >> 16, 1u);
+			const u32 scaleH = std::max((u32(objMtx.BaseScaleY) * 0x40 * objSpriteScaleH) >> 16, 1u);
 			if (gs_s2dexversion == eVer1_3) {
 				// XH = AND ((((objX << 0x10) * 0x0800 * (0x80007FFF/BaseScaleX)) >> 0x30) + X + A2) by B0
 				// XL = XH + AND (((((imageW - A1) * 0x100) *  (0x80007FFF/scaleW)) >> 0x20) + B2) by B0
@@ -669,6 +669,15 @@ void gSPObjLoadTxtr(u32 tx)
 	const u32 address = RSP_SegmentToPhysical(tx);
 	uObjTxtr *objTxtr = (uObjTxtr*)&RDRAM[address];
 
+	// sid is a byte offset into the four word gSP.status, read straight out of
+	// an RDRAM structure, so sid >> 2 reaches 16383. Both the test below and
+	// the write at the end of this function index with it.
+	if (objTxtr->block.sid > 12) {
+		DebugMsg(DEBUG_NORMAL | DEBUG_ERROR,
+			"// gSPObjLoadTxtr: invalid sid %u\n", objTxtr->block.sid);
+		return;
+	}
+
 	if ((gSP.status[objTxtr->block.sid >> 2] & objTxtr->block.mask) != objTxtr->block.flag) {
 		switch (objTxtr->block.type) {
 			case G_OBJLT_TXTRBLOCK:
@@ -1566,6 +1575,12 @@ void S2DEX_Select_DL(u32 w0, u32 w1)
 	const u8 sid = gSP.selectDL.sid;
 	const u32 flag = gSP.selectDL.flag;
 	const u32 mask = w1;
+
+	if (sid >= 4) {
+		DebugMsg(DEBUG_NORMAL | DEBUG_ERROR, "// S2DEX_Select_DL: invalid sid %u\n", sid);
+		return;
+	}
+
 	if ((gSP.status[sid] & mask) == flag)
 		// Do nothing;
 		return;

src/uCodes/ZSort.cpp

@@ -11,6 +11,9 @@
 #include "3DMath.h"
 #include "DisplayWindow.h"
 
+// ZSort addresses DMEM with a 0x400 bias baked into the display list fields.
+static const u32 ZSORT_DMEM_BIAS = 1024;
+
 #define	GZM_USER0		0
 #define	GZM_USER1		2
 #define	GZM_MMTX		4
@@ -192,7 +195,20 @@ void ZSort_XFMLight( u32 _w0, u32 _w1 )
 {
 	int mid = _SHIFTR(_w0, 0, 8);
 	gSPNumLights(1 + _SHIFTR(_w1, 12, 8));
-	u32 addr = -1024 + _SHIFTR(_w1, 0, 12);
+	// ZSort DMEM offsets carry a 0x400 bias. The subtraction is unsigned, so a
+	// field below 1024 wraps to near UINT32_MAX and is then used as a DMEM
+	// index. The walk below skips 8 bytes, then strides 24 per light for
+	// numLights lights and twice more for the lookat vectors, reading up to
+	// offset +10 (+3 more once the ^3 byte swap is applied) each time.
+	const u32 addrField = _SHIFTR(_w1, 0, 12);
+	const u32 addrExtent = 8 + (gSP.numLights + 2) * 24 + 14;
+	if (addrField < ZSORT_DMEM_BIAS ||
+		!isDMEMRangeValid(addrField - ZSORT_DMEM_BIAS, addrExtent)) {
+		LOG(LOG_ERROR, "ZSort_XFMLight: DMEM offset 0x%03x with %u lights is out of range",
+			addrField, gSP.numLights);
+		return;
+	}
+	u32 addr = addrField - ZSORT_DMEM_BIAS;
 
 	assert(mid == GZM_MMTX);
 /*
@@ -244,12 +260,37 @@ void ZSort_LightingL( u32, u32 )
 
 void ZSort_Lighting( u32 _w0, u32 _w1 )
 {
-	u32 csrs = -1024 + _SHIFTR(_w0, 12, 12);
-	u32 nsrs = -1024 + _SHIFTR(_w0, 0, 12);
+	const u32 csrsField = _SHIFTR(_w0, 12, 12);
+	const u32 nsrsField = _SHIFTR(_w0, 0, 12);
 	u32 num = 1 + _SHIFTR(_w1, 24, 8);
-	u32 cdest = -1024 + _SHIFTR(_w1, 12, 12);
-	u32 tdest = -1024 + _SHIFTR(_w1, 0, 12);
+	const u32 cdestField = _SHIFTR(_w1, 12, 12);
+	const u32 tdestField = _SHIFTR(_w1, 0, 12);
+
+	// All four offsets carry the 0x400 bias and the subtraction is unsigned, so
+	// a field below 1024 wraps to near UINT32_MAX. cdest and tdest are DMEM
+	// *write* indices. Per vertex this reads 3 bytes at nsrs, 4 at csrs, and
+	// writes 4 at cdest and two s16 at tdest, with num up to 256.
+	if (csrsField < ZSORT_DMEM_BIAS || nsrsField < ZSORT_DMEM_BIAS ||
+		cdestField < ZSORT_DMEM_BIAS || tdestField < ZSORT_DMEM_BIAS) {
+		LOG(LOG_ERROR, "ZSort_Lighting: DMEM offset below the 0x400 bias");
+		return;
+	}
+	u32 csrs = csrsField - ZSORT_DMEM_BIAS;
+	u32 nsrs = nsrsField - ZSORT_DMEM_BIAS;
+	u32 cdest = cdestField - ZSORT_DMEM_BIAS;
+	u32 tdest = tdestField - ZSORT_DMEM_BIAS;
+	// use_material compares the biased value against 0x0ff0, which a 12 bit
+	// field can never produce after subtracting 1024. Left as it is: correcting
+	// the sentinel would change which vertices get material colours.
 	int use_material = (csrs != 0x0ff0);
+	if (!isDMEMRangeValid(nsrs, num * 3) ||
+		(use_material && !isDMEMRangeValid(csrs, num * 4)) ||
+		!isDMEMRangeValid(cdest, num * 4) ||
+		!isDMEMRangeValid(tdest, num * 4)) {
+		LOG(LOG_ERROR, "ZSort_Lighting: %u vertices from DMEM 0x%03x/0x%03x to 0x%03x/0x%03x is out of range",
+			num, nsrs, csrs, cdest, tdest);
+		return;
+	}
 	tdest >>= 1;
 	GraphicsDrawer & drawer = dwnd().getDrawer();
 	drawer.setDMAVerticesSize(num);

src/uCodes/ZSortBOSS.cpp

@@ -1,5 +1,6 @@
 #include <assert.h>
 #include <math.h>
+#include <algorithm>
 #include "N64.h"
 #include "RSP.h"
 #include "RDP.h"
@@ -140,7 +141,7 @@ void StoreMatrix( f32 mtx[4][4], u32 address )
 void ZSortBOSS_MoveMem( u32 _w0, u32 _w1 )
 {
 	int flag = (_w0 >> 23) & 0x01;
-	int len = 1 + (_w0 >> 12) & 0x7ff;
+	u32 len = 1 + ((_w0 >> 12) & 0x7ff);
 	u32 addr = RSP_SegmentToPhysical(_w1);
 	assert((addr & 3) == 0);
 	assert((_w0 & 3) == 0);
@@ -217,15 +218,27 @@ void ZSortBOSS_MoveMem( u32 _w0, u32 _w1 )
 		return;
 	}
 
-	if((_w0 & 0xfff) == 0x730) {
-		assert(len == 256);
-		memcpy(gstate.fogtable, (RDRAM + addr), len);
+	// Everything below copies len bytes between RDRAM and DMEM at offsets taken
+	// from the display list. len reaches 2048, the DMEM offset 4095 and DMEM is
+	// only 4 KB, so both ends need checking.
+	const u32 dmemOffset = _w0 & 0xfff;
+	if (!isRDRAMRangeValid(addr, len) || !isDMEMRangeValid(dmemOffset, len)) {
+		LOG(LOG_ERROR, "ZSortBOSS_MoveMem: %u bytes between RDRAM 0x%08x and DMEM 0x%04x is out of range",
+			len, addr, dmemOffset);
+		return;
+	}
+
+	if (dmemOffset == 0x730) {
+		assert(len == sizeof(gstate.fogtable));
+		// No return here on purpose: the fall-through below also mirrors the
+		// table into DMEM, which is what the microcode expects.
+		memcpy(gstate.fogtable, (RDRAM + addr), std::min(len, u32(sizeof(gstate.fogtable))));
 	}
 
-	if(flag == 0) {
-		memcpy((DMEM + (_w0 & 0xfff)), (RDRAM + addr), len);
+	if (flag == 0) {
+		memcpy((DMEM + dmemOffset), (RDRAM + addr), len);
 	} else {
-		memcpy((RDRAM + addr), (DMEM + (_w0 & 0xfff)), len);
+		memcpy((RDRAM + addr), (DMEM + dmemOffset), len);
 	}
 }
 
@@ -582,7 +595,12 @@ void ZSortBOSS_TransformLights( u32 _w0, u32 _w1 )
 
 	M44 *mtx = nullptr;
 	int addr = _w1 & 0xfff;
-	gSP.numLights = 1 - (_w1 >> 12);
+	const s32 numLights = 1 - static_cast<s32>(_w1 >> 12);
+	if (numLights < 0 || numLights >= 12) {
+		LOG(LOG_ERROR, "ZSortBOSS_TransformLights: invalid light count %d from 0x%08x", numLights, _w1);
+		return;
+	}
+	gSP.numLights = static_cast<u32>(numLights);
 
 	/*
 	switch(_w0 & 0xfff) {