Merge pull request #22893 from hrydgard/flip-listener-race

Debugger: Fix a use-after-free and a deadlock in GPU stats; one connection per command in adhoc_session.py

Core/Debugger/WebSocket/GPUStatsSubscriber.cpp

@@ -15,6 +15,7 @@
 // Official git repository and contact information can be found at
 // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/.
 
+#include <atomic>
 #include <mutex>
 #include <vector>
 
@@ -82,8 +83,9 @@ struct WebSocketGPUStatsState : public DebuggerSubscriber {
 
 protected:
 	bool forced_ = false;
-	bool sendNext_ = false;
-	bool sendFeed_ = false;
+	// Read by FlipListener() on the CPU thread before it takes pendingLock_.
+	std::atomic<bool> sendNext_{ false };
+	std::atomic<bool> sendFeed_{ false };
 
 	std::string lastTicket_;
 	std::mutex pendingLock_;
@@ -186,12 +188,14 @@ void WebSocketGPUStatsState::Feed(DebuggerRequest &req) {
 	if (!req.ParamBool("enable", &enable, DebuggerParamType::OPTIONAL))
 		return;
 
-	std::lock_guard<std::mutex> guard(pendingLock_);
-	sendFeed_ = enable;
+	// Not under pendingLock_: the CPU thread takes it in FlipListener(), so holding it across
+	// Core_RunOnCPUThread() deadlocks. forced_ is only touched on this connection's thread.
 	if (forced_ != enable) {
 		Core_RunOnCPUThread([enable] { PSP_ForceDebugStats(enable); });
 		forced_ = enable;
 	}
+	std::lock_guard<std::mutex> guard(pendingLock_);
+	sendFeed_ = enable;
 }
 
 void WebSocketGPUStatsState::Broadcast(net::WebSocketServer *ws) {

Core/HW/Display.cpp

@@ -250,15 +250,14 @@ void DisplayFireVblankEnd() {
 }
 
 void DisplayFireFlip() {
-	std::vector<FlipListener> toCall = [] {
-		std::lock_guard<std::mutex> guard(listenersLock);
-		return flipListeners;
-	}();
-
 	// This is also the right time to calculate FPS.
 	CalculateFPS();
 
-	for (FlipListener cb : toCall) {
+	// Held across the calls, so that once __DisplayForgetFlip() returns (on a debugger connection's
+	// thread, say) its listener isn't running and won't be called again. A listener must not add or
+	// remove listeners.
+	std::lock_guard<std::mutex> guard(listenersLock);
+	for (const FlipListener &cb : flipListeners) {
 		cb.first(cb.second);
 	}
 }
@@ -281,7 +280,10 @@ void __DisplayForgetFlip(FlipCallback callback, void *userdata) {
 
 // This is called on game bootup.
 void DisplayHWInit() {
-	flipListeners.clear();
+	{
+		std::lock_guard<std::mutex> guard(listenersLock);
+		flipListeners.clear();
+	}
 	frameStartTicks = 0;
 	numVBlanks = 0;
 	isVblank = 0;

Tools/adhoc_session.py

@@ -58,6 +58,8 @@ class Debugger:
 		self.send("broadcast.config.set", disallowed={"logger": not logs, "input": True})
 
 	def close(self):
+		if getattr(self, "shared", False):
+			return
 		try:
 			self.sock.close()
 		except OSError:
@@ -145,6 +147,21 @@ class Debugger:
 			self.wait_for(lambda m: m.get("event") == "cpu.resume")
 
 
+# One connection per instance for the whole command. `wait` polls every few milliseconds, and a
+# connection per poll means thousands of them, each with its own thread and subscribers on the
+# PPSSPP side.
+_connections = {}
+
+
+def connect(inst):
+	dbg = _connections.get(inst["port"])
+	if dbg is None:
+		dbg = Debugger(inst["port"])
+		dbg.shared = True
+		_connections[inst["port"]] = dbg
+	return dbg
+
+
 def session_file(args):
 	return os.path.join(args.dir, "session.json")
 
@@ -168,7 +185,7 @@ def emulated_seconds(session):
 	# The slowest instance's clock: the one a script is waiting for.
 	times = []
 	for inst in session["instances"]:
-		dbg = Debugger(inst["port"])
+		dbg = connect(inst)
 		times.append(dbg.request("cpu.status")["us"] / 1e6)
 		dbg.close()
 	return min(times)
@@ -318,7 +335,7 @@ def cmd_press(args):
 		if t > 0:
 			emulated_sleep(session, args.every)
 		for inst in selected(session, args.i):
-			dbg = Debugger(inst["port"])
+			dbg = connect(inst)
 			# The answer only comes once the press is over, so don't wait for it.
 			dbg.send("input.buttons.press", button=args.button, duration=args.frames)
 			dbg.close()
@@ -340,7 +357,7 @@ def cmd_type(args):
 
 	def press(button):
 		for inst in insts:
-			dbg = Debugger(inst["port"])
+			dbg = connect(inst)
 			dbg.send("input.buttons.press", button=button, duration=3)
 			dbg.close()
 		emulated_sleep(session, 0.15)
@@ -373,12 +390,12 @@ def cmd_stick(args):
 	session = load_session(args)
 	insts = selected(session, args.i)
 	for inst in insts:
-		dbg = Debugger(inst["port"])
+		dbg = connect(inst)
 		dbg.request("input.analog.send", x=args.x, y=args.y)
 		dbg.close()
 	emulated_sleep(session, args.seconds)
 	for inst in insts:
-		dbg = Debugger(inst["port"])
+		dbg = connect(inst)
 		dbg.request("input.analog.send", x=0, y=0)
 		dbg.close()
 
@@ -391,7 +408,7 @@ def grab_png(inst, attempts=3):
 	# An instance busy loading can be slow to answer; try again rather than end a long script.
 	for attempt in range(attempts):
 		try:
-			dbg = Debugger(inst["port"])
+			dbg = connect(inst)
 			dbg.stop_cpu()
 			answer = dbg.request("gpu.buffer.screenshot", type="uri")
 			dbg.resume_cpu()
@@ -428,7 +445,7 @@ def cmd_shot(args):
 def cmd_stats(args):
 	session = load_session(args)
 	for inst in selected(session, args.i):
-		dbg = Debugger(inst["port"])
+		dbg = connect(inst)
 		status = dbg.request("cpu.status")
 		stats = dbg.request("gpu.stats.get", timeout=5)
 		dbg.close()
@@ -487,7 +504,7 @@ def cmd_until(args):
 			sys.exit("until: instances %s never matched; their screens are in until_failed_N.png" % ",".join(str(i["n"]) for i in waiting))
 		if args.press and (last_press is None or now - last_press >= args.every):
 			for inst in waiting:
-				dbg = Debugger(inst["port"])
+				dbg = connect(inst)
 				dbg.send("input.buttons.press", button=args.press, duration=6)
 				dbg.close()
 			last_press = now
@@ -509,7 +526,7 @@ def cmd_log(args):
 		print("log: verbose messages are compiled out (MAX_LOGLEVEL in Common/Log.h); this shows debug and above", file=sys.stderr)
 	session = load_session(args)
 	for inst in selected(session, args.i):
-		dbg = Debugger(inst["port"])
+		dbg = connect(inst)
 		dbg.request("log.channel.set", channel=args.channel, level=args.level, enabled=True)
 		dbg.close()