Merge pull request #22062 from hrydgard/fix-data-audit-bugs

Common audit: Fix /data bugs

Common/Data/Collections/CharQueue.h

@@ -188,6 +188,10 @@ public:
 
 	// If return value is negative, one wasn't found.
 	int next_crlf_offset() {
+		// A trailing '\r' with no '\n' after it yet (e.g. right at a TCP fragmentation
+		// boundary) is a normal "not found yet", not an error - don't peek() past the
+		// data we actually have.
+		const size_t totalSize = size();
 		int offset = 0;
 		Block *b = head_;
 		do {
@@ -195,7 +199,7 @@ public:
 			for (int i = 0; i < remain; i++) {
 				if (b->data[b->head + i] == '\r') {
 					// Use peek to avoid handling edge cases.
-					if (peek(offset + i + 1) == '\n') {
+					if ((size_t)(offset + i + 1) < totalSize && peek(offset + i + 1) == '\n') {
 						return offset + i;
 					}
 				}

Common/Data/Collections/FastVec.h

@@ -89,9 +89,13 @@ public:
 	// Limited functionality for inserts and similar, add as needed.
 	T &insert(T *iter) {
 		int pos = iter - data_;
+		// Capture the old size before ExtendByOne() bumps size_ - the move should only
+		// cover the elements that actually existed before this insert, not size_ + 1
+		// worth (which would read/write one element past the valid old range).
+		int oldSize = (int)size_;
 		ExtendByOne();
-		if (pos + 1 < (int)size_) {
-			memmove(data_ + pos + 1, data_ + pos, (size_ - pos) * sizeof(T));
+		if (pos < oldSize) {
+			memmove(data_ + pos + 1, data_ + pos, (oldSize - pos) * sizeof(T));
 		}
 		return data_[pos];
 	}

Common/Data/Encoding/Shiftjis.h

@@ -43,6 +43,11 @@ struct ShiftJIS {
 		}
 
 		// Okay, if we didn't return, it's time for the second byte (the cell.)
+		if (c_[index_] == 0) {
+			// Truncated sequence right at the end of the string - don't consume the
+			// terminator, or index_ would end up one past it (OOB on the next call).
+			return INVALID;
+		}
 		j = (uint8_t)c_[index_++];
 		// Not a valid second byte.
 		if (j < 0x40 || j == 0x7F || j >= 0xFD) {

Common/Data/Format/JSONReader.cpp

@@ -106,7 +106,12 @@ bool JsonGet::getStringVector(std::vector<std::string> *vec) const {
 }
 
 double JsonGet::getFloat(const char *child_name) const {
-	return get(child_name, JSON_NUMBER)->value.toNumber();
+	const JsonNode *val = get(child_name, JSON_NUMBER);
+	if (!val) {
+		ERROR_LOG(Log::IO, "Number '%s' missing from node", child_name);
+		return 0.0;
+	}
+	return val->value.toNumber();
 }
 
 double JsonGet::getFloat(const char *child_name, double default_value) const {
@@ -117,7 +122,12 @@ double JsonGet::getFloat(const char *child_name, double default_value) const {
 }
 
 int JsonGet::getInt(const char *child_name) const {
-	return (int)get(child_name, JSON_NUMBER)->value.toNumber();
+	const JsonNode *val = get(child_name, JSON_NUMBER);
+	if (!val) {
+		ERROR_LOG(Log::IO, "Number '%s' missing from node", child_name);
+		return 0;
+	}
+	return (int)val->value.toNumber();
 }
 
 int JsonGet::getInt(const char *child_name, int default_value) const {
@@ -128,7 +138,12 @@ int JsonGet::getInt(const char *child_name, int default_value) const {
 }
 
 bool JsonGet::getBool(const char *child_name) const {
-	return get(child_name)->value.getTag() == JSON_TRUE;
+	const JsonNode *val = get(child_name);
+	if (!val) {
+		ERROR_LOG(Log::IO, "Value '%s' missing from node", child_name);
+		return false;
+	}
+	return val->value.getTag() == JSON_TRUE;
 }
 
 bool JsonGet::getBoolOr(const char *child_name, bool default_value) const {

Common/Data/Format/RIFF.cpp

@@ -90,7 +90,19 @@ void RIFFReader::Ascend() {
 }
 
 void RIFFReader::ReadData(void *what, int count) {
-	memcpy(what, data_ + pos_, count);
+	if (count > 0) {
+		int available = pos_ < fileSize_ ? fileSize_ - pos_ : 0;
+		int toRead = count < available ? count : available;
+		if (toRead > 0) {
+			memcpy(what, data_ + pos_, toRead);
+		}
+		if (toRead < count) {
+			// Truncated/corrupt file - don't read past the buffer. Zero the rest so
+			// callers don't read uninitialized data.
+			ERROR_LOG(Log::IO, "RIFFReader::ReadData: wanted %d bytes but only %d available", count, toRead);
+			memset((uint8_t *)what + toRead, 0, count - toRead);
+		}
+	}
 	pos_ += count;
 	count &= 3;
 	if (count) {

Common/GPU/Vulkan/VulkanQueueRunner.cpp

@@ -43,7 +43,7 @@ RenderPassType MergeRPTypes(RenderPassType a, RenderPassType b) {
 }
 
 void VulkanQueueRunner::CreateDeviceObjects() {
-	INFO_LOG(Log::G3D, "VulkanQueueRunner::CreateDeviceObjects");
+	DEBUG_LOG(Log::G3D, "VulkanQueueRunner::CreateDeviceObjects");
 
 	RPKey key{
 		VKRRenderPassLoadAction::CLEAR, VKRRenderPassLoadAction::CLEAR, VKRRenderPassLoadAction::CLEAR,
@@ -67,7 +67,7 @@ void VulkanQueueRunner::CreateDeviceObjects() {
 }
 
 void VulkanQueueRunner::DestroyDeviceObjects() {
-	INFO_LOG(Log::G3D, "VulkanQueueRunner::DestroyDeviceObjects");
+	DEBUG_LOG(Log::G3D, "VulkanQueueRunner::DestroyDeviceObjects");
 
 	syncReadback_.Destroy(vulkan_);
 

Common/Net/HTTPClient.cpp

@@ -141,7 +141,7 @@ bool Connection::Connect(int maxTries, double timeout, bool *cancelConnect) {
 					if (!unreachable) {
 						ERROR_LOG(Log::HTTP, "connect(%d) call to %s failed (%d: %s)", sock, addrStr, errorCode, errorString.c_str());
 					} else {
-						INFO_LOG(Log::HTTP, "connect(%d): Ignoring unreachable resolved address %s", sock, addrStr);
+						VERBOSE_LOG(Log::HTTP, "connect(%d): Ignoring unreachable resolved address %s", sock, addrStr);
 					}
 					closesocket(sock);
 					continue;

Common/Render/Text/draw_text_sdl.cpp

@@ -292,13 +292,13 @@ void TextDrawerSDL::SetOrCreateFont(const FontStyle &style) {
 	uint8_t *fileData = nullptr;
 	std::string useFont = GetFilenameForFontStyle(style) + ".ttf";
 	const int ptSize = static_cast<int>(style.sizePts / dpiScale_ * 1.25f);
-	INFO_LOG(Log::G3D, "Loading SDL font '%s' from VFS at size %d pts", useFont.c_str(), ptSize);
+	DEBUG_LOG(Log::G3D, "Loading SDL font '%s' from VFS at size %d pts", useFont.c_str(), ptSize);
 
 	size_t fileSz;
 	fileData = g_VFS.ReadFile(useFont.c_str(), &fileSz);
 	if (fileData) {
 		SDL_IOStream *rw = SDL_IOFromConstMem(fileData, fileSz);
-		INFO_LOG(Log::G3D, "Opened font from RW: '%p' '%d'", fileData, (int)fileSz);
+		DEBUG_LOG(Log::G3D, "Opened font from RW: '%p' '%d'", fileData, (int)fileSz);
 		font = TTF_OpenFontIO(rw, true, static_cast<float>(ptSize));
 		if (!font) {
 			ERROR_LOG(Log::G3D, "Failed to load font from asset file: '%s'", useFont.c_str());

Core/Config.cpp

@@ -1464,7 +1464,7 @@ bool Config::Save(const char *saveReason) {
 				}
 				if (!ShouldSaveSetting(meta.settings[j].GetVoidPtr(configBlock))) {
 					// Skip settings marked as "don't save".
-					INFO_LOG(Log::Config, "Not saving setting '%.*s' as marked as don't save.", STR_VIEW(meta.settings[j].IniKey()));
+					DEBUG_LOG(Log::Config, "Not saving setting '%.*s' as marked as don't save.", STR_VIEW(meta.settings[j].IniKey()));
 					continue;
 				}
 				meta.settings[j].WriteToIniSection(configBlock, section);

Core/ELF/ElfReader.cpp

@@ -560,7 +560,7 @@ int ElfReader::LoadInto(u32 loadAddress, bool fromTop) {
 			}
 		}
 	}
-	memblock.ListBlocks();
+	memblock.ListBlocks(LogLevel::LDEBUG);
 
 	DEBUG_LOG(Log::Loader, "%d sections:", header->e_shnum);
 

Core/HLE/sceKernelMemory.cpp

@@ -371,18 +371,18 @@ void __KernelMemoryDoState(PointerWrap &p)
 void __KernelMemoryShutdown()
 {
 #ifdef _DEBUG
-	INFO_LOG(Log::sceKernel, "Shutting down volatile memory pool: ");
-	volatileMemory.ListBlocks();
+	DEBUG_LOG(Log::sceKernel, "Shutting down volatile memory pool");
+	volatileMemory.ListBlocks(LogLevel::LDEBUG);
 #endif
 	volatileMemory.Shutdown();
 #ifdef _DEBUG
-	INFO_LOG(Log::sceKernel,"Shutting down user memory pool: ");
-	userMemory.ListBlocks();
+	DEBUG_LOG(Log::sceKernel,"Shutting down user memory pool");
+	userMemory.ListBlocks(LogLevel::LDEBUG);
 #endif
 	userMemory.Shutdown();
 #ifdef _DEBUG
-	INFO_LOG(Log::sceKernel,"Shutting down \"kernel\" memory pool: ");
-	kernelMemory.ListBlocks();
+	DEBUG_LOG(Log::sceKernel,"Shutting down \"kernel\" memory pool");
+	kernelMemory.ListBlocks(LogLevel::LDEBUG);
 #endif
 	kernelMemory.Shutdown();
 	tlsplThreadEndChecks.clear();
@@ -823,7 +823,7 @@ public:
 			else
 				address = alloc->Alloc(size, type == PSP_SMEM_High, name);
 #ifdef _DEBUG
-			alloc->ListBlocks();
+			alloc->ListBlocks(LogLevel::LDEBUG);
 #endif
 		}
 	}
@@ -1914,7 +1914,7 @@ SceUID sceKernelCreateTlspl(const char *name, u32 partition, u32 attr, u32 block
 	u32 totalSize = alignedSize * count;
 	u32 blockPtr = allocator->Alloc(totalSize, (attr & PSP_TLSPL_ATTR_HIGHMEM) != 0, StringFromFormat("TLS/%s", name).c_str());
 #ifdef _DEBUG
-	allocator->ListBlocks();
+	allocator->ListBlocks(LogLevel::LDEBUG);
 #endif
 
 	if (blockPtr == (u32)-1)

Core/Util/BlockAllocator.cpp

@@ -134,7 +134,7 @@ u32 BlockAllocator::AllocAligned(u32 &size, u32 sizeGrain, u32 grain, bool fromT
 	}
 
 	//Out of memory :(
-	ListBlocks();
+	ListBlocks(LogLevel::LINFO);
 	ERROR_LOG(Log::sceKernel, "Block Allocator (%08x-%08x) failed to allocate %i (%08x) bytes of contiguous memory", rangeStart_, rangeStart_ + rangeSize_, size, size);
 	return -1;
 }
@@ -215,7 +215,7 @@ u32 BlockAllocator::AllocAt(u32 position, u32 size, const char *tag)
 
 
 	//Out of memory :(
-	ListBlocks();
+	ListBlocks(LogLevel::LINFO);
 	ERROR_LOG(Log::sceKernel, "Block Allocator (%08x-%08x) failed to allocate %i (%08x) bytes of contiguous memory", rangeStart_, rangeStart_ + rangeSize_, alignedSize, alignedSize);
 	return -1;
 }
@@ -387,15 +387,14 @@ u32 BlockAllocator::GetBlockSizeFromAddress(u32 addr) const
 		return -1;
 }
 
-void BlockAllocator::ListBlocks() const
-{
-	DEBUG_LOG(Log::sceKernel,"-----------");
+void BlockAllocator::ListBlocks(LogLevel level) const {
+	GENERIC_LOG(Log::sceKernel, level, "-----------");
 	for (const Block *bp = bottom_; bp != NULL; bp = bp->next)
 	{
 		const Block &b = *bp;
-		DEBUG_LOG(Log::sceKernel, "Block: %08x - %08x size %08x taken=%i tag=%s", b.start, b.start+b.size, b.size, b.taken ? 1:0, b.tag);
+		GENERIC_LOG(Log::sceKernel, level, "Block: %08x - %08x size %08x taken=%i tag=%s", b.start, b.start+b.size, b.size, b.taken ? 1:0, b.tag);
 	}
-	DEBUG_LOG(Log::sceKernel,"-----------");
+	GENERIC_LOG(Log::sceKernel, level, "-----------");
 }
 
 u32 BlockAllocator::GetLargestFreeBlockSize() const

Core/Util/BlockAllocator.h

@@ -21,6 +21,8 @@ class PointerWrap;
 
 #include "Common/CommonTypes.h"
 
+#include "Common/Log.h"
+
 class BlockAllocator
 {
 public:
@@ -30,7 +32,7 @@ public:
 	void Init(u32 _rangeStart, u32 _rangeSize, bool suballoc);
 	void Shutdown();
 
-	void ListBlocks() const;
+	void ListBlocks(LogLevel level) const;
 
 	// WARNING: size can be modified upwards!
 	u32 Alloc(u32 &size, bool fromTop = false, const char *tag = 0);

GPU/Common/ReplacedTexture.cpp

@@ -649,27 +649,33 @@ ReplacedTexture::LoadLevelResult ReplacedTexture::LoadLevelData(VFSFileReference
 		}
 		vfs_->CloseFile(openFile);
 
-		int w, h, f;
-		uint8_t *image;
+		// LoadZIMPtr writes to these as arrays (one entry per mip level, up to
+		// ZIM_MAX_MIP_LEVELS) whenever the file has ZIM_HAS_MIPS set - passing plain
+		// scalars here was an OOB stack write waiting for a mipped (or malicious) ZIM.
+		int w[ZIM_MAX_MIP_LEVELS], h[ZIM_MAX_MIP_LEVELS], f;
+		uint8_t *image[ZIM_MAX_MIP_LEVELS];
 		std::vector<uint8_t> &out = data_[mipLevel];
 		// TODO: Zim files can actually hold mipmaps (although no tool has ever been made to create them :P)
-		if (LoadZIMPtr(&zim[0], fileSize, &w, &h, &f, &image)) {
-			if (w > level.w || h > level.h) {
+		// We only use the first level for now.
+		int numLevels = LoadZIMPtr(&zim[0], fileSize, w, h, &f, image);
+		if (numLevels > 0) {
+			if (w[0] > level.w || h[0] > level.h) {
 				ERROR_LOG(Log::TexReplacement, "Texture replacement changed since header read: %s", filename.c_str());
+				free(image[0]);
 				return LoadLevelResult::LOAD_ERROR;
 			}
 
 			out.resize(level.w * level.h * 4);
-			if (w == level.w) {
-				memcpy(&out[0], image, level.w * 4 * level.h);
+			if (w[0] == level.w) {
+				memcpy(&out[0], image[0], level.w * 4 * level.h);
 			} else {
-				for (int y = 0; y < h; ++y) {
-					memcpy(&out[level.w * 4 * y], image + w * 4 * y, w * 4);
+				for (int y = 0; y < h[0]; ++y) {
+					memcpy(&out[level.w * 4 * y], image[0] + w[0] * 4 * y, w[0] * 4);
 				}
 			}
-			free(image);
+			free(image[0]);
 
-			const TextureAlpha res = CheckAlpha32Rect((u32 *)&out[0], level.w, w, h, 0xFF000000);
+			const TextureAlpha res = CheckAlpha32Rect((u32 *)&out[0], level.w, w[0], h[0], 0xFF000000);
 			if (res == TextureAlpha::Any || mipLevel == 0) {
 				alphaStatus_ = res;
 			}

SDL/SDLJoystick.cpp

@@ -25,11 +25,10 @@ SDLJoystick::SDLJoystick(bool init_SDL ) : registeredAsEventHandler(false) {
 	}
 
 	const char *dbPath = "gamecontrollerdb.txt";
-	INFO_LOG(Log::System, "loading control pad mappings from %s:", dbPath);
-
 	size_t size;
 	u8 *mappingData = g_VFS.ReadFile(dbPath, &size);
 	if (mappingData) {
+		DEBUG_LOG(Log::System, "loading control pad mappings from '%s'", dbPath);
 		SDL_IOStream *io = SDL_IOFromConstMem(mappingData, size);
 		if (SDL_AddGamepadMappingsFromIO(io, true) == -1) {
 			ERROR_LOG(Log::System, "Failed to read mapping data - corrupt?");

SDL/SDLMain.cpp

@@ -1521,7 +1521,6 @@ static void ProcessSDLEvent(SDL_Window *window, const SDL_Event &event, InputSta
 		switch (event.button.button) {
 		case SDL_BUTTON_LEFT:
 			{
-				INFO_LOG(Log::UI, "SDL_EVENT_MOUSE_BUTTON_DOWN: %f x %f", event.button.x, event.button.y);
 				// We have to juggle around 3 kinds of "DPI spaces" if a logical DPI is
 				// provided (through --dpi, it is equal to system DPI if unspecified):
 				// - SDL gives us motion events in "system DPI" points

UI/BackgroundAudio.cpp

@@ -107,37 +107,42 @@ bool WavData::Read(RIFFReader &file_) {
 		if (file_.Descend('smpl')) {
 			std::vector<u8> smplData;
 			smplData.resize(file_.GetCurrentChunkSize());
-			file_.ReadData(&smplData[0], (int)smplData.size());
+			if (!smplData.empty()) {
+				file_.ReadData(smplData.data(), (int)smplData.size());
+			}
 
-			int numLoops = *(int *)&smplData[28];
-			struct AtracLoopInfo {
-				int cuePointID;
-				int type;
-				int startSample;
-				int endSample;
-				int fraction;
-				int playCount;
-			};
+			// A short/corrupt 'smpl' chunk shouldn't make us read past the buffer.
+			if (smplData.size() >= 32) {
+				int numLoops = *(int *)&smplData[28];
+				struct AtracLoopInfo {
+					int cuePointID;
+					int type;
+					int startSample;
+					int endSample;
+					int fraction;
+					int playCount;
+				};
 
-			if (numLoops > 0 && smplData.size() >= 36 + sizeof(AtracLoopInfo) * numLoops) {
-				AtracLoopInfo *loops = (AtracLoopInfo *)&smplData[36];
-				int samplesPerFrame = codec == PSP_CODEC_AT3PLUS ? 2048 : 1024;
+				if (numLoops > 0 && smplData.size() >= 36 + sizeof(AtracLoopInfo) * numLoops) {
+					AtracLoopInfo *loops = (AtracLoopInfo *)&smplData[36];
+					int samplesPerFrame = codec == PSP_CODEC_AT3PLUS ? 2048 : 1024;
 
-				for (int i = 0; i < numLoops; ++i) {
-					// Only seen forward loops, so let's ignore others.
-					if (loops[i].type != 0)
-						continue;
+					for (int i = 0; i < numLoops; ++i) {
+						// Only seen forward loops, so let's ignore others.
+						if (loops[i].type != 0)
+							continue;
 
-					// We ignore loop interpolation (fraction) and play count for now.
-					raw_offset_loop_start = (loops[i].startSample / samplesPerFrame) * raw_bytes_per_frame;
-					loop_start_offset = loops[i].startSample % samplesPerFrame;
-					raw_offset_loop_end = (loops[i].endSample / samplesPerFrame) * raw_bytes_per_frame;
-					loop_end_offset = loops[i].endSample % samplesPerFrame;
+						// We ignore loop interpolation (fraction) and play count for now.
+						raw_offset_loop_start = (loops[i].startSample / samplesPerFrame) * raw_bytes_per_frame;
+						loop_start_offset = loops[i].startSample % samplesPerFrame;
+						raw_offset_loop_end = (loops[i].endSample / samplesPerFrame) * raw_bytes_per_frame;
+						loop_end_offset = loops[i].endSample % samplesPerFrame;
 
-					if (loops[i].playCount == 0) {
-						// This was an infinite loop, so ignore the rest.
-						// In practice, there's usually only one and it's usually infinite.
-						break;
+						if (loops[i].playCount == 0) {
+							// This was an infinite loop, so ignore the rest.
+							// In practice, there's usually only one and it's usually infinite.
+							break;
+						}
 					}
 				}
 			}

UI/NativeApp.cpp

@@ -223,6 +223,8 @@ class GlobalListener : public ControlListener {
 				g_Config.bShowImDebugger = !g_Config.bShowImDebugger;
 			}
 			break;
+		default:
+			break;
 		}
 	}
 };
@@ -801,7 +803,6 @@ void NativeInit(int argc, const char *argv[], const CommandLineOptions &cmdLineO
 
 	ApplyAchievementsHostOverride();
 
-	DEBUG_LOG(Log::System, "ScreenManager!");
 	g_screenManager = new ScreenManager();
 	if (g_Config.memStickDirectory.empty()) {
 		INFO_LOG(Log::System, "No memstick directory! Asking for one to be configured.");
@@ -1203,6 +1204,8 @@ void NativeFrame(GraphicsContext *graphicsContext) {
 			case QueuedEventType::TOUCH:
 				ImGui_ImplPlatform_TouchEvent(event.touch);
 				break;
+			default:
+				break;
 			}
 		}
 	}