Merge pull request #22186 from hrydgard/medium-correctness-fixes

Medium-severity correctness fixes

Common/ExceptionHandlerSetup.cpp

@@ -312,6 +312,28 @@ static struct sigaction old_sa_bus;
 static stack_t old_signal_stack{};
 static bool old_signal_stack_valid = false;
 
+// Hand the signal on to whatever was installed before us. Returning from a fault handler just
+// re-runs the faulting instruction, so for anything we can't deal with this is the only exit
+// that isn't an infinite loop.
+static void ChainToPreviousHandler(int sig, siginfo_t *info, void *raw_context) {
+	struct sigaction *old_sa = sig == SIGSEGV ? &old_sa_segv : &old_sa_bus;
+	// Per the sigaction man page: with SA_SIGINFO it's sa_sigaction, otherwise sa_handler is
+	// SIG_DFL, SIG_IGN, or a handler pointer.
+	if (old_sa->sa_flags & SA_SIGINFO) {
+		old_sa->sa_sigaction(sig, info, raw_context);
+		return;
+	}
+	if (old_sa->sa_handler == SIG_DFL) {
+		signal(sig, SIG_DFL);
+		return;
+	}
+	if (old_sa->sa_handler == SIG_IGN) {
+		// Ignore signal
+		return;
+	}
+	old_sa->sa_handler(sig);
+}
+
 static void sigsegv_handler(int sig, siginfo_t* info, void* raw_context) {
 	if (sig != SIGSEGV && sig != SIGBUS) {
 		// We are not interested in other signals - handle it as usual.
@@ -320,7 +342,11 @@ static void sigsegv_handler(int sig, siginfo_t* info, void* raw_context) {
 	ucontext_t* context = (ucontext_t*)raw_context;
 	int sicode = info->si_code;
 	if (sicode != SEGV_MAPERR && sicode != SEGV_ACCERR) {
-		// Huh? Return.
+		// Not an address fault we can do anything with - an MTE, protection-key or shadow
+		// stack fault, or a signal sent with kill(). Returning here would re-run the
+		// faulting instruction forever at 100% CPU, and would also swallow the signal from
+		// whatever was installed before us (a crash reporter, say).
+		ChainToPreviousHandler(sig, info, raw_context);
 		return;
 	}
 	uintptr_t bad_address = (uintptr_t)info->si_addr;
@@ -346,26 +372,7 @@ static void sigsegv_handler(int sig, siginfo_t* info, void* raw_context) {
 		// SIG_IGN: The signal is ignored
 		// Any other value is a function pointer to a signal handler
 
-		struct sigaction* old_sa;
-		if (sig == SIGSEGV) {
-			old_sa = &old_sa_segv;
-		} else {
-			old_sa = &old_sa_bus;
-		}
-
-		if (old_sa->sa_flags & SA_SIGINFO) {
-			old_sa->sa_sigaction(sig, info, raw_context);
-			return;
-		}
-		if (old_sa->sa_handler == SIG_DFL) {
-			signal(sig, SIG_DFL);
-			return;
-		}
-		if (old_sa->sa_handler == SIG_IGN) {
-			// Ignore signal
-			return;
-		}
-		old_sa->sa_handler(sig);
+		ChainToPreviousHandler(sig, info, raw_context);
 	}
 }
 

Common/MemoryUtil.cpp

@@ -145,8 +145,17 @@ void *AllocateExecutableMemory(size_t size) {
 #endif
 		if (ptr) {
 			ptr = VirtualAlloc(ptr, aligned_size, MEM_RESERVE | MEM_COMMIT, prot);
+			if (!ptr) {
+				// Finding a free region isn't the same as being able to reserve it: VirtualAlloc
+				// rounds a non-null address down to the 64K allocation granularity, and
+				// SearchForFreeMem only returns page alignment, so the rounded-down base can land
+				// back inside something committed. Another thread allocating in between does it too.
+				WARN_LOG(Log::Common, "Could not reserve the nearby executable memory found for jit. Proceeding with far memory.");
+			}
 		} else {
 			WARN_LOG(Log::Common, "Unable to find nearby executable memory for jit. Proceeding with far memory.");
+		}
+		if (!ptr) {
 			// Can still run, thanks to "RipAccessible".
 			ptr = VirtualAlloc(nullptr, aligned_size, MEM_RESERVE | MEM_COMMIT, prot);
 		}

Common/TimeUtil.cpp

@@ -238,12 +238,12 @@ double time_now_d() {
 }
 
 uint64_t time_now_raw() {
-	struct timeval tv;
-	gettimeofday(&tv, nullptr);
-	if (start == 0) {
-		start = tv.tv_sec;
-	}
-	return (double)tv.tv_sec + (double)tv.tv_usec * (1.0 / micros);
+	// Nanoseconds, like the other platforms - from_time_raw() scales by 1/nanos. This used to
+	// build a double of seconds and return it through the uint64_t, so it both lost the fraction
+	// and was off by a factor of a billion.
+	struct timespec tp;
+	clock_gettime(CLOCK_MONOTONIC, &tp);
+	return (uint64_t)tp.tv_sec * 1000000000ULL + tp.tv_nsec;
 }
 
 double from_time_raw(uint64_t raw_time) {
@@ -257,7 +257,10 @@ double from_time_raw_relative(uint64_t raw_time) {
 void yield() {}
 
 double time_now_unix_utc() {
-	return time_now_raw();
+	// Not time_now_raw() - that's a monotonic clock with no relation to the epoch.
+	struct timeval tv;
+	gettimeofday(&tv, nullptr);
+	return (double)tv.tv_sec + (double)tv.tv_usec * (1.0 / micros);
 }
 
 double time_to_unix_utc(double t) {
@@ -267,10 +270,12 @@ double time_to_unix_utc(double t) {
 }
 
 Instant::Instant() {
-	struct timeval tv;
-	gettimeofday(&tv, nullptr);
-	nativeStart_ = tv.tv_sec;
-	nsecs_ = tv.tv_usec;
+	// Has to be the same clock, and the same unit, as ElapsedNanos() below: this took the wall
+	// clock in microseconds while that one subtracts it from a monotonic clock in nanoseconds.
+	struct timespec ts;
+	clock_gettime(CLOCK_MONOTONIC, &ts);
+	nativeStart_ = ts.tv_sec;
+	nsecs_ = ts.tv_nsec;
 }
 
 int64_t Instant::ElapsedNanos() const {
@@ -278,12 +283,12 @@ int64_t Instant::ElapsedNanos() const {
 	clock_gettime(CLOCK_MONOTONIC, &ts);
 
 	int64_t secs = ts.tv_sec - nativeStart_;
-	int64_t usecs = ts.tv_nsec - nsecs_;
-	if (usecs < 0) {
+	int64_t nsecs = ts.tv_nsec - nsecs_;
+	if (nsecs < 0) {
 		secs--;
-		usecs += 1000000;
+		nsecs += 1000000000;
 	}
-	return secs * 1000000000 + usecs * 1000;
+	return secs * 1000000000 + nsecs;
 }
 
 double Instant::ElapsedSeconds() const {

Core/Core.cpp

@@ -913,7 +913,7 @@ void Core_MemoryExceptionHLE(MIPSState *mips, u32 address, u32 accessSize, Memor
 	// We try to derive the reason here, though maybe it should be passed in explicitly?
 	// TODO: This check should probably be added to regular memory accesses too.
 	if (Memory::IsValidAddress(address)) {
-		if (accessSize == 2 || accessSize == 4 || accessSize == 8 || (address & (accessSize - 1))) {
+		if ((accessSize == 2 || accessSize == 4 || accessSize == 8) && (address & (accessSize - 1))) {
 			extra = " (unaligned)";
 		} else if (accessSize > 8 && (accessSize & 3)) {
 			extra = " (unaligned struct)";

Core/Instance.cpp

@@ -22,6 +22,7 @@
 #include <unistd.h>
 #include <sys/types.h>
 #include <sys/mman.h>
+#include <sys/file.h>
 #include <sys/stat.h>
 #include <fcntl.h>
 #endif
@@ -96,10 +97,16 @@ static bool UpdateInstanceCounter(void (*callback)(volatile InstanceInfo *)) {
 	}
 
 	bool result = false;
-	if (mlock(buf, BUF_SIZE) == 0) {
+	// An actual advisory lock on the shm object. This used to call mlock(), which only pins
+	// pages in RAM and provides no mutual exclusion whatsoever - two instances starting at the
+	// same moment could both read the counter and come away with the same PPSSPP_ID, then both
+	// believe they were the first instance and write the config over each other.
+	if (flock(hIDMapFile, LOCK_EX) == 0) {
 		callback(buf);
-		munlock(buf, BUF_SIZE);
+		flock(hIDMapFile, LOCK_UN);
 		result = true;
+	} else {
+		ERROR_LOG(Log::sceNet, "flock(%s) failure: %s", ID_SHM_NAME, GetLastErrorMsg().c_str());
 	}
 
 	munmap(buf, BUF_SIZE);
@@ -157,7 +164,14 @@ void InitInstanceCounter() {
 #endif
 
 	bool success = UpdateInstanceCounter([](volatile InstanceInfo *buf) {
-		PPSSPP_ID = ++buf->next;
+		// The shared segment outlives the processes that used it (see the shm_unlink comment),
+		// so next keeps climbing across runs and eventually wraps this uint8_t. ID 0 is not a
+		// valid instance - it fails the IsFirstInstance() check, which quietly disables config
+		// saving - so skip past it.
+		if (++buf->next == 0) {
+			buf->next = 1;
+		}
+		PPSSPP_ID = buf->next;
 		buf->total++;
 	});
 	if (!success) {

Core/PSPLoaders.cpp

@@ -160,11 +160,21 @@ void InitMemorySizeForGame() {
 		}
 
 		if (umdData.empty()) {
-			std::vector<u8> umdDataBin;
-			// .data() rather than &umdDataBin[0] - the file can legitimately be empty, and
-			// indexing an empty vector is undefined.
-			if (pspFileSystem.ReadEntireFile("disc0:/UMD_DATA.BIN", umdDataBin) >= 0) {
-				umdData = std::string((const char *)umdDataBin.data(), umdDataBin.size());
+			// A real UMD_DATA.BIN is a few dozen bytes - it's just the disc ID line matched below.
+			// Check the size first: this comes off the disc image, which is not something we trust,
+			// and ReadEntireFile has no limit of its own - it would resize a vector to whatever the
+			// image claims, and the string copy after it doubles that.
+			const s64 MAX_UMD_DATA_SIZE = 4096;
+			const PSPFileInfo info = pspFileSystem.GetFileInfo("disc0:/UMD_DATA.BIN");
+			if (info.exists && info.size > MAX_UMD_DATA_SIZE) {
+				WARN_LOG(Log::Loader, "Ignoring implausibly large UMD_DATA.BIN (%lld bytes)", (long long)info.size);
+			} else if (info.exists) {
+				std::vector<u8> umdDataBin;
+				// .data() rather than &umdDataBin[0] - the file can legitimately be empty, and
+				// indexing an empty vector is undefined.
+				if (pspFileSystem.ReadEntireFile("disc0:/UMD_DATA.BIN", umdDataBin) >= 0) {
+					umdData = std::string((const char *)umdDataBin.data(), umdDataBin.size());
+				}
 			}
 		}
 

GPU/Common/FramebufferManagerCommon.cpp

@@ -2627,7 +2627,7 @@ bool FramebufferManagerCommon::NotifyBlockTransferBefore(u32 dstBasePtr, int dst
 
 	if (srcRect.channel == RASTER_DEPTH) {
 		// Ignore the found buffer if it's not 16-bit - we create a new more suitable one instead.
-		if (dstRect.channel == RASTER_COLOR && dstRect.vfb->fb_format == GE_FORMAT_8888) {
+		if (dstBuffer && dstRect.channel == RASTER_COLOR && dstRect.vfb->fb_format == GE_FORMAT_8888) {
 			dstBuffer = false;
 		}
 	}

GPU/Common/ReplacedTexture.cpp

@@ -106,7 +106,10 @@ ReplacedTexture::~ReplacedTexture() {
 	}
 
 	for (auto &level : levels_) {
-		vfs_->ReleaseFile(level.fileRef);
+		// Null when replacement was switched off after we were cached - see NotifyConfigChanged.
+		if (vfs_) {
+			vfs_->ReleaseFile(level.fileRef);
+		}
 		level.fileRef = nullptr;
 	}
 }
@@ -201,7 +204,11 @@ inline uint32_t RoundUpTo4(uint32_t value) {
 }
 
 void ReplacedTexture::Prepare(VFSBackend *vfs) {
-	_assert_(vfs != nullptr);
+	if (!vfs) {
+		// Replacement was switched off while this was queued. Nothing to load from any more.
+		SetState(ReplacementState::NOT_FOUND);
+		return;
+	}
 
 	this->vfs_ = vfs;
 

GPU/Common/TextureReplacer.cpp

@@ -109,6 +109,12 @@ void TextureReplacer::NotifyConfigChanged() {
 	}
 
 	if (!replaceEnabled_ && wasReplaceEnabled) {
+		// Everything in levelCache_ holds this pointer - LoadIni fixes them up when it swaps the
+		// VFS, and the same has to happen here or they're left dangling. Decimate(ALL) below only
+		// frees their data, it doesn't erase the entries.
+		for (auto &repl : levelCache_) {
+			repl.second->vfs_ = nullptr;
+		}
 		delete vfs_;
 		vfs_ = nullptr;
 		Decimate(ReplacerDecimateMode::ALL);
@@ -178,6 +184,14 @@ bool TextureReplacer::LoadIni(std::string *error, bool notify) {
 		if (ini.GetOrCreateSection("games")->Get(gameID_.c_str(), &overrideFilename)) {
 			if (overrideFilename == "true") {
 				// Ignore it
+			} else if (HasParentDirComponent(overrideFilename)) {
+				// Same reason the [hashes] filenames are checked: for a directory-backed pack,
+				// DirectoryReader resolves this against the pack directory, so "../.." reaches
+				// anything on disk. Third-party packs are just downloads.
+				*error = "Override ini name must stay inside the pack: '" + overrideFilename + "'";
+				ERROR_LOG(Log::TexReplacement, "%s", error->c_str());
+				delete dir;
+				return false;
 			} else if (!overrideFilename.empty() && overrideFilename != INI_FILENAME) {
 				IniFile overrideIni;
 				iniLoaded = overrideIni.LoadFromVFS(*dir, overrideFilename);