Merge pull request #21380 from hrydgard/last-fixes

iOS: Try to avoid leaking file sandbox bookmarks, avoid trying to install zips where it's not possible

Common/File/FileUtil.cpp

@@ -1495,6 +1495,12 @@ bool IsProbablyInDownloadsFolder(const Path &filename) {
 	default:
 		break;
 	}
+#if PPSSPP_PLATFORM(IOS)
+	if (filename.FilePathContainsNoCase("com~apple~CloudDocs")) {
+		return true;
+	}
+#endif
+
 	return filename.FilePathContainsNoCase("download");
 }
 

Common/System/System.h

@@ -62,6 +62,7 @@ void System_LaunchUrl(LaunchUrlType urlType, std::string_view url);
 enum class UIEventNotification {
 	MENU_RETURN,
 	POPUP_CLOSED,
+	DIALOG_CLOSED,
 	TEXT_GOTFOCUS,
 	TEXT_LOSTFOCUS,
 };

Common/UI/UIScreen.cpp

@@ -264,6 +264,10 @@ void UIDialogScreen::sendMessage(UIMessage message, const char *value) {
 	}
 }
 
+UIDialogScreen::~UIDialogScreen() {
+	System_NotifyUIEvent(UIEventNotification::DIALOG_CLOSED);
+}
+
 bool UIScreen::IsOnTop() const {
 	return screenManager()->topScreen() == this;
 }

Common/UI/UIScreen.h

@@ -91,6 +91,7 @@ private:
 class UIDialogScreen : public UIScreen {
 public:
 	UIDialogScreen() : UIScreen(), finished_(false) {}
+	~UIDialogScreen() override;
 	bool key(const KeyInput &key) override;
 	void sendMessage(UIMessage message, const char *value) override;
 

Core/FileLoaders/LocalFileLoader.cpp

@@ -21,6 +21,7 @@
 #include "Common/Log.h"
 #include "Common/File/FileUtil.h"
 #include "Common/File/DirListing.h"
+#include "Core/Util/DarwinFileSystemServices.h"
 #include "Core/FileLoaders/LocalFileLoader.h"
 
 #if PPSSPP_PLATFORM(ANDROID)
@@ -77,23 +78,37 @@ LocalFileLoader::LocalFileLoader(const Path &filename)
 	}
 #endif
 
+
 #if defined(HAVE_LIBRETRO_VFS)
 	file_ = File::OpenCFile(filename, "rb");
 	if (!file_) {
+		ERROR_LOG(Log::FileSystem, "LocalFileLoader: failed to open file: '%s'", filename.c_str());
 		return;
 	}
 	filesize_ = File::GetFileSize(file_);
+#elif PPSSPP_PLATFORM(IOS)
+	if (!File::Exists(filename)) {
+		// Try to "unlock" the path before the file loader hits it
+		Path newFilename = DarwinFileSystemServices::reauthorizeBookmarkByPath(filename);
+		if (!newFilename.empty()) {
+			filename_ = newFilename;
+		}
+	}
+	fd_ = open(filename_.c_str(), O_RDONLY | O_CLOEXEC);
+	if (fd_ == -1) {
+		ERROR_LOG(Log::FileSystem, "LocalFileLoader: failed to open file: '%s'", filename_.c_str());
+		return;
+	}
+	DetectSizeFd();
 #elif !defined(_WIN32)
-
 	fd_ = open(filename.c_str(), O_RDONLY | O_CLOEXEC);
 	if (fd_ == -1) {
+		ERROR_LOG(Log::FileSystem, "LocalFileLoader: failed to open file: '%s'", filename.c_str());
 		return;
 	}
 
 	DetectSizeFd();
-
 #else // _WIN32
-
 	const DWORD access = GENERIC_READ, share = FILE_SHARE_READ, mode = OPEN_EXISTING, flags = FILE_ATTRIBUTE_NORMAL;
 #if PPSSPP_PLATFORM(UWP)
 	handle_ = CreateFile2FromAppW(filename.ToWString().c_str(), access, share, mode, nullptr);
@@ -121,6 +136,9 @@ LocalFileLoader::~LocalFileLoader() {
 	if (file_ != nullptr) {
 		fclose(file_);
 	}
+#elif PPSSPP_PLATFORM(IOS)
+	close(fd_);
+	DarwinFileSystemServices::stopAccessingPath(filename_);
 #elif !defined(_WIN32)
 	if (fd_ != -1) {
 		close(fd_);

Core/FileLoaders/ZipFileLoader.cpp

@@ -46,19 +46,45 @@ ZipFileLoader::~ZipFileLoader() {
 bool ZipFileLoader::Initialize(int fileIndex) {
 	_dbg_assert_(!data_);
 
+	if (!zipArchive_) {
+		ERROR_LOG(Log::IO, "Cannot initialize: ZIP archive is null");
+		return false;
+	}
+
 	struct zip_stat zstat;
 	int retval = zip_stat_index(zipArchive_, fileIndex, ZIP_FL_NOCASE | ZIP_FL_UNCHANGED, &zstat);
 	if (retval < 0) {
 		return false;
 	}
 	const char *name = zip_get_name(zipArchive_, fileIndex, ZIP_FL_NOCASE | ZIP_FL_UNCHANGED);
+	if (!name) {
+		ERROR_LOG(Log::IO, "Failed to get name for file index %d", fileIndex);
+		return false;
+	}
 	fileExtension_ = KeepIncludingLast(name, '.');
 
 	_dbg_assert_(zstat.index == fileIndex);
 	dataFileSize_ = zstat.size;
 	dataFile_ = zip_fopen_index(zipArchive_, zstat.index, ZIP_FL_UNCHANGED);
+	if (!dataFile_) {
+		ERROR_LOG(Log::IO, "Failed to open file index %d from ZIP", fileIndex);
+		return false;
+	}
+
+	// Handle zero-size files
+	if (dataFileSize_ == 0) {
+		data_ = nullptr;
+		return true;
+	}
+
 	data_ = (u8 *)malloc(dataFileSize_);
-	return data_ != nullptr;
+	if (!data_) {
+		ERROR_LOG(Log::IO, "Failed to allocate %lld bytes for ZIP file data", dataFileSize_);
+		zip_fclose(dataFile_);
+		dataFile_ = nullptr;
+		return false;
+	}
+	return true;
 }
 
 size_t ZipFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flags flags) {
@@ -67,7 +93,6 @@ size_t ZipFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flags fl
 	}
 
 	if (absolutePos + (s64)bytes > dataFileSize_) {
-		// TODO: This could go negative..
 		bytes = dataFileSize_ - absolutePos;
 	}
 
@@ -78,7 +103,14 @@ size_t ZipFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flags fl
 			remaining = (int)(dataFileSize_ - dataReadPos_);
 		}
 		zip_int64_t retval = zip_fread(dataFile_, data_ + dataReadPos_, remaining);
-		_dbg_assert_(retval == remaining);
+		if (retval < 0) {
+			ERROR_LOG(Log::IO, "zip_fread failed with error code %lld", retval);
+			return 0;
+		}
+		if (retval != remaining) {
+			ERROR_LOG(Log::IO, "zip_fread: expected %d bytes, got %lld", remaining, retval);
+			return 0;
+		}
 		dataReadPos_ += retval;
 	}
 
@@ -96,36 +128,32 @@ zip_int64_t ZipFileLoader::ZipSourceCallback(void *data, zip_uint64_t len, zip_s
 	}
 	case ZIP_SOURCE_READ:
 	{
-		size_t readBytes = static_cast<zip_int64_t>(backend_->ReadAt(zipReadPos_, len, data));
+		size_t readBytes = backend_->ReadAt(zipReadPos_, len, data);
 		zipReadPos_ += readBytes;
-		return readBytes;
+		return static_cast<zip_int64_t>(readBytes);
 	}
 	case ZIP_SOURCE_SEEK:
 	{
-		struct SeekData {
-			zip_int64_t offset;
-			int whence;
-		};
-		if (len < sizeof(SeekData)) {
-			return -1; // Invalid argument size
+		if (len < sizeof(zip_source_args_seek_t)) {
+			return -1;
 		}
-		const SeekData *seekData = static_cast<const SeekData*>(data);
+		const zip_source_args_seek_t *seekArgs = (const zip_source_args_seek_t *)data;
 		zip_int64_t new_offset;
-		switch (seekData->whence) {
+		switch (seekArgs->whence) {
 		case SEEK_SET:
-			new_offset = seekData->offset;
+			new_offset = seekArgs->offset;
 			break;
 		case SEEK_CUR:
-			new_offset = zipReadPos_ + seekData->offset;
+			new_offset = zipReadPos_ + seekArgs->offset;
 			break;
 		case SEEK_END:
-			new_offset = backend_->FileSize() + seekData->offset;
+			new_offset = backend_->FileSize() + seekArgs->offset;
 			break;
 		default:
-			return -1; // Invalid 'whence' value
+			return -1;
 		}
 		if (new_offset < 0 || new_offset > backend_->FileSize()) {
-			return -1; // Offset out of bounds
+			return -1;
 		}
 		zipReadPos_ = new_offset;
 		return 0;
@@ -150,7 +178,7 @@ zip_int64_t ZipFileLoader::ZipSourceCallback(void *data, zip_uint64_t len, zip_s
 	case ZIP_SOURCE_FREE:
 		return 0;
 	case ZIP_SOURCE_SUPPORTS:
-		return zip_source_make_command_bitmap(ZIP_SOURCE_READ, ZIP_SOURCE_SEEK, ZIP_SOURCE_TELL, ZIP_SOURCE_OPEN, ZIP_SOURCE_CLOSE, ZIP_SOURCE_STAT);
+		return zip_source_make_command_bitmap(ZIP_SOURCE_READ, ZIP_SOURCE_SEEK, ZIP_SOURCE_TELL, ZIP_SOURCE_OPEN, ZIP_SOURCE_CLOSE, ZIP_SOURCE_STAT, -1);
 	default:
 		return -1;
 	}

Core/Loaders.cpp

@@ -58,15 +58,7 @@ FileLoader *ConstructFileLoader(const Path &filename) {
 		}
 		return new CachingFileLoader(baseLoader);
 	}
-	#if PPSSPP_PLATFORM(IOS)
-	if (!File::Exists(filename)) {
-		// Try to "unlock" the path before the file loader hits it
-		Path newFilename = DarwinFileSystemServices::reauthorizeBookmarkByPath(filename);
-		if (!newFilename.empty()) {
-			return new LocalFileLoader(newFilename);
-		}
-	}
-	#endif
+
 	return new LocalFileLoader(filename);
 }
 

Core/Util/DarwinFileSystemServices.mm

@@ -70,8 +70,8 @@ static void addBookmark(NSURL *url);
 	BOOL success = [url startAccessingSecurityScopedResource];
 	if (success) {
 		// 2. Pass the path to PPSSPP
-		// Note: In a real app, you'd want to call stopAccessingSecurityScopedResource
-		// when the game is closed, otherwise you "leak" kernel permissions.
+		// You should call stopAccessingSecurityScopedResource
+		// when the file is closed, otherwise you "leak" kernel permissions.
 		self.panelCallback(true, Path(url.path.UTF8String));
 	} else {
 		ERROR_LOG(Log::System, "Failed to start accessing security scoped resource for: %s", url.path.UTF8String);
@@ -139,6 +139,7 @@ static void addBookmark(NSURL *url) {
 		if (accessStarted) [url stopAccessingSecurityScopedResource];
 	}
 }
+
 Path DarwinFileSystemServices::reauthorizeBookmarkByPath(const Path &pathStr) {
     INFO_LOG(Log::System, "Darwin: Reauthorizing [%s]", pathStr.c_str());
 
@@ -207,7 +208,7 @@ void DarwinFileSystemServices::stopAccessingPath(const Path &pathStr) {
         [url stopAccessingSecurityScopedResource];
 
         activeAccessTokens.erase(it);
-    }
+	}
 }
 
 void DarwinFileSystemServices::terminate() {

Core/WebServer.cpp

@@ -191,6 +191,10 @@ static std::string RemotePathForRecent(const std::string &filename) {
 }
 
 static Path LocalFromRemotePath(std::string_view path) {
+	// This happens for some reason...
+	if (startsWith(path, "//")) {
+		path.remove_prefix(1);
+	}
 	switch ((RemoteISOShareType)g_Config.iRemoteISOShareType) {
 	case RemoteISOShareType::RECENT:
 		for (const std::string &filename : g_recentFiles.GetRecentFiles()) {

GPU/Common/FramebufferManagerCommon.cpp

@@ -2201,6 +2201,7 @@ bool FramebufferManagerCommon::NotifyFramebufferCopy(u32 src, u32 dst, int size,
 		// awkward visual artefacts.
 		const u8 *srcBase = Memory::GetPointerUnchecked(src);
 		GEBufferFormat srcFormat = channel == RASTER_DEPTH ? GE_FORMAT_DEPTH16 : dstBuffer->fb_format;
+		// TODO: srcStride here looks suspicious! Actually the whole calculation does...
 		int srcStride = channel == RASTER_DEPTH ? dstBuffer->z_stride : dstBuffer->fb_stride;
 		DrawPixels(dstBuffer, 0, dstY, srcBase, srcFormat, srcStride, dstBuffer->width, dstH, channel, "MemcpyFboUpload_DrawPixels");
 		SetColorUpdated(dstBuffer, skipDrawReason);

GPU/GPUCommonHW.cpp

@@ -798,6 +798,8 @@ void GPUCommonHW::InvalidateCache(u32 addr, int size, GPUInvalidationType type)
 }
 
 bool GPUCommonHW::FramebufferDirty() {
+	if (!framebufferManager_)
+		return true;
 	VirtualFramebuffer *vfb = framebufferManager_->GetDisplayVFB();
 	if (vfb) {
 		bool dirty = vfb->dirtyAfterDisplay;
@@ -808,6 +810,8 @@ bool GPUCommonHW::FramebufferDirty() {
 }
 
 bool GPUCommonHW::FramebufferReallyDirty() {
+	if (!framebufferManager_)
+		return true;
 	VirtualFramebuffer *vfb = framebufferManager_->GetDisplayVFB();
 	if (vfb) {
 		bool dirty = vfb->reallyDirtyAfterDisplay;

UI/InstallZipScreen.cpp

@@ -117,6 +117,16 @@ void InstallZipScreen::CreateSettingsViews(UI::ViewGroup *parent) {
 	}
 }
 
+template<class T>
+bool Contains(const std::vector<T> &vec, const T &needle) {
+	for (const auto &item : vec) {
+		if (item == needle) {
+			return true;
+		}
+	}
+	return false;
+}
+
 void InstallZipScreen::CreateContentViews(UI::ViewGroup *parent) {
 	using namespace UI;
 
@@ -161,9 +171,13 @@ void InstallZipScreen::CreateContentViews(UI::ViewGroup *parent) {
 				destFolders_.push_back(parent);
 			}
 			if (g_Config.currentDirectory.IsLocalType() && File::Exists(g_Config.currentDirectory) && g_Config.currentDirectory != parent) {
-				destFolders_.push_back(g_Config.currentDirectory);
+				if (!Contains(destFolders_, g_Config.currentDirectory)) {
+					destFolders_.push_back(g_Config.currentDirectory);
+				}
+			}
+			if (!Contains(destFolders_, g_Config.memStickDirectory)) {
+				destFolders_.push_back(g_Config.memStickDirectory);
 			}
-			destFolders_.push_back(g_Config.memStickDirectory);
 		} else {
 			destFolders_.push_back(GetSysDirectory(DIRECTORY_GAME));
 		}

UI/MainScreen.cpp

@@ -163,15 +163,20 @@ static void DrawIconWithText(UIContext &dc, ImageID image, std::string_view text
 		dc.SetFontStyle(dc.GetTheme().uiFont);
 		dc.PopScissor();
 	} else {
-		bool scissor = false;
-		if (tw + 150 > bounds.w) {
-			dc.PushScissor(bounds);
-			scissor = true;
-		}
 		dc.Draw()->DrawImage(image, bounds.x + 72, bounds.centerY(), 0.88f * (gridStyle ? g_Config.fGameGridScale : 1.0), style.fgColor, ALIGN_CENTER);
-		dc.DrawText(text, bounds.x + 150, bounds.centerY(), style.fgColor, ALIGN_VCENTER | FLAG_WRAP_TEXT);
 
-		if (scissor) {
+		float tx = 150;
+		int availableWidth = bounds.w - 150;
+		float sineWidth = std::max(0.0f, (tw - availableWidth)) / 2.0f;
+		if (availableWidth < tw) {
+			tx -= (1.0f + sin(time_now_d() * 1.5f)) * sineWidth;
+			Bounds tb = bounds;
+			tb.x = bounds.x + 150;
+			tb.w = availableWidth;
+			dc.PushScissor(tb);
+		}
+		dc.DrawText(text, bounds.x + tx, bounds.centerY(), style.fgColor, ALIGN_VCENTER | FLAG_WRAP_TEXT);
+		if (availableWidth < tw) {
 			dc.PopScissor();
 		}
 	}
@@ -295,6 +300,9 @@ void GameButton::Draw(UIContext &dc) {
 	if (down_) {
 		style = dc.GetTheme().itemDownStyle;
 	}
+	if (startsWith(ginfo->GetTitle(), "Nddemo")) {
+		style = style;
+	}
 
 	// Some types we just draw a default icon for.
 	ImageID imageIcon = ImageID::invalid();
@@ -427,61 +435,61 @@ void GameButton::Draw(UIContext &dc) {
 		if (overlayColor) {
 			dc.FillRect(Drawable(overlayColor), overlayBounds);
 		}
-	}
-
-	char discNumInfo[8];
-	if (ginfo->disc_total > 1)
-		snprintf(discNumInfo, sizeof(discNumInfo), "-DISC%d", ginfo->disc_number);
-	else
-		discNumInfo[0] = '\0';
-
-	dc.Draw()->Flush();
-	dc.RebindTexture();
-	dc.SetFontStyle(dc.GetTheme().uiFont);
-	if (!gridStyle_) {
-		float tw, th;
-		dc.Draw()->Flush();
-		dc.PushScissor(bounds_);
-		const std::string currentTitle = ginfo->GetTitle();
-		if (!currentTitle.empty()) {
-			title_ = ReplaceAll(currentTitle, "\n", " ");
-		}
-
-		dc.MeasureText(dc.GetFontStyle(), 1.0f, 1.0f, title_, &tw, &th, 0);
-
-		int availableWidth = bounds_.w - 150;
-		if (g_Config.bShowIDOnGameIcon) {
-			float vw, vh;
-			dc.MeasureText(dc.GetFontStyle(), 0.7f, 0.7f, ginfo->id_version, &vw, &vh, 0);
-			availableWidth -= vw + 20;
-			dc.SetFontScale(0.7f, 0.7f);
-			dc.DrawText(ginfo->id_version, bounds_.x + availableWidth + 160, bounds_.centerY(), style.fgColor, ALIGN_VCENTER);
-			dc.SetFontScale(1.0f, 1.0f);
-		}
-		float sineWidth = std::max(0.0f, (tw - availableWidth)) / 2.0f;
-
-		float tx = 150;
-		if (availableWidth < tw) {
-			tx -= (1.0f + sin(time_now_d() * 1.5f)) * sineWidth;
-			Bounds tb = bounds_;
-			tb.x = bounds_.x + 150;
-			tb.w = availableWidth;
-			dc.PushScissor(tb);
-		}
-		dc.DrawText(title_, bounds_.x + tx, bounds_.centerY(), style.fgColor, ALIGN_VCENTER);
-		if (availableWidth < tw) {
-			dc.PopScissor();
-		}
-		dc.Draw()->Flush();
-		dc.PopScissor();
-	} else if (!texture) {
-		dc.Draw()->Flush();
-		dc.PushScissor(bounds_);
-		dc.DrawText(title_, bounds_.x + 4, bounds_.centerY(), style.fgColor, ALIGN_VCENTER);
-		dc.Draw()->Flush();
-		dc.PopScissor();
 	} else {
+		char discNumInfo[8];
+		if (ginfo->disc_total > 1)
+			snprintf(discNumInfo, sizeof(discNumInfo), "-DISC%d", ginfo->disc_number);
+		else
+			discNumInfo[0] = '\0';
+
 		dc.Draw()->Flush();
+		dc.RebindTexture();
+		dc.SetFontStyle(dc.GetTheme().uiFont);
+		if (!gridStyle_) {
+			float tw, th;
+			dc.Draw()->Flush();
+			dc.PushScissor(bounds_);
+			const std::string currentTitle = ginfo->GetTitle();
+			if (!currentTitle.empty()) {
+				title_ = ReplaceAll(currentTitle, "\n", " ");
+			}
+
+			dc.MeasureText(dc.GetFontStyle(), 1.0f, 1.0f, title_, &tw, &th, 0);
+
+			int availableWidth = bounds_.w - 150;
+			if (g_Config.bShowIDOnGameIcon) {
+				float vw, vh;
+				dc.MeasureText(dc.GetFontStyle(), 0.7f, 0.7f, ginfo->id_version, &vw, &vh, 0);
+				availableWidth -= vw + 20;
+				dc.SetFontScale(0.7f, 0.7f);
+				dc.DrawText(ginfo->id_version, bounds_.x + availableWidth + 160, bounds_.centerY(), style.fgColor, ALIGN_VCENTER);
+				dc.SetFontScale(1.0f, 1.0f);
+			}
+			float sineWidth = std::max(0.0f, (tw - availableWidth)) / 2.0f;
+
+			float tx = 150;
+			if (availableWidth < tw) {
+				tx -= (1.0f + sin(time_now_d() * 1.5f)) * sineWidth;
+				Bounds tb = bounds_;
+				tb.x = bounds_.x + 150;
+				tb.w = availableWidth;
+				dc.PushScissor(tb);
+			}
+			dc.DrawText(title_, bounds_.x + tx, bounds_.centerY(), style.fgColor, ALIGN_VCENTER);
+			if (availableWidth < tw) {
+				dc.PopScissor();
+			}
+			dc.Draw()->Flush();
+			dc.PopScissor();
+		} else if (!texture) {
+			dc.Draw()->Flush();
+			dc.PushScissor(bounds_);
+			dc.DrawText(title_, bounds_.x + 4, bounds_.centerY(), style.fgColor, ALIGN_VCENTER);
+			dc.Draw()->Flush();
+			dc.PopScissor();
+		} else {
+			dc.Draw()->Flush();
+		}
 	}
 
 	if (ginfo->hasConfig && !ginfo->id.empty()) {

ios/ViewControllerCommon.mm

@@ -209,6 +209,7 @@
 	}
 
 	[picker dismissViewControllerAnimated:YES completion:nil];
+
 	[self hideKeyboard];
 }
 

ios/main.mm

@@ -533,6 +533,7 @@ bool System_MakeRequest(SystemRequestType type, int requestId, const std::string
 	{
 		switch ((UIEventNotification)param3) {
 		case UIEventNotification::POPUP_CLOSED:
+		case UIEventNotification::DIALOG_CLOSED:
 			[sharedViewController hideKeyboard];
 			break;
 		case UIEventNotification::TEXT_GOTFOCUS: