From f2cdbeace6dd30b25d006a21d080a5ba69075f48 Mon Sep 17 00:00:00 2001
From: Max Kellermann <max@musicpd.org>
Date: Thu, 22 Aug 2019 09:07:41 +0200
Subject: [PATCH] Revert "Client: eliminate SetExpired(), call Close()
 directly"

This reverts commit 58d7804d666896600d0af1de067f21c14414bf46.  It
caused a use-after-free bug when Client::OnSocketError() was called
due to a failed write, e.g. if the output buffer was full.
---
 src/client/Client.hxx  | 10 ++++++++--
 src/client/Event.cxx   |  4 ++--
 src/client/Expire.cxx  | 21 ++++++++++++++++++---
 src/client/Idle.cxx    |  3 +++
 src/client/Process.cxx |  7 ++++++-
 src/client/Read.cxx    |  5 +++++
 src/client/Write.cxx   |  7 +++++++
 7 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/src/client/Client.hxx b/src/client/Client.hxx
index 11ee0ed00..564b2a01a 100644
--- a/src/client/Client.hxx
+++ b/src/client/Client.hxx
@@ -122,9 +122,15 @@ public:
 
 	using FullyBufferedSocket::GetEventLoop;
 
-	void Close() noexcept;
+	gcc_pure
+	bool IsExpired() const noexcept {
+		return !FullyBufferedSocket::IsDefined();
+	}
 
-	using FullyBufferedSocket::Write;
+	void Close() noexcept;
+	void SetExpired() noexcept;
+
+	bool Write(const void *data, size_t length) noexcept;
 
 	/**
 	 * Write a null-terminated string.
diff --git a/src/client/Event.cxx b/src/client/Event.cxx
index 4f5d8eb43..d231a020e 100644
--- a/src/client/Event.cxx
+++ b/src/client/Event.cxx
@@ -25,11 +25,11 @@ Client::OnSocketError(std::exception_ptr ep) noexcept
 {
 	FormatError(ep, "error on client %d", num);
 
-	Close();
+	SetExpired();
 }
 
 void
 Client::OnSocketClosed() noexcept
 {
-	Close();
+	SetExpired();
 }
diff --git a/src/client/Expire.cxx b/src/client/Expire.cxx
index 8c450e1ef..bf094acc4 100644
--- a/src/client/Expire.cxx
+++ b/src/client/Expire.cxx
@@ -18,16 +18,31 @@
  */
 
 #include "Client.hxx"
+#include "BackgroundCommand.hxx"
 #include "Domain.hxx"
 #include "Log.hxx"
 
+void
+Client::SetExpired() noexcept
+{
+	if (IsExpired())
+		return;
+
+	background_command.reset();
+
+	FullyBufferedSocket::Close();
+	timeout_event.Schedule(std::chrono::steady_clock::duration::zero());
+}
+
 void
 Client::OnTimeout() noexcept
 {
-	assert(!idle_waiting);
-	assert(!background_command);
+	if (!IsExpired()) {
+		assert(!idle_waiting);
+		assert(!background_command);
 
-	FormatDebug(client_domain, "[%u] timeout", num);
+		FormatDebug(client_domain, "[%u] timeout", num);
+	}
 
 	Close();
 }
diff --git a/src/client/Idle.cxx b/src/client/Idle.cxx
index 62e77eb05..f8987b33a 100644
--- a/src/client/Idle.cxx
+++ b/src/client/Idle.cxx
@@ -54,6 +54,9 @@ Client::IdleNotify() noexcept
 void
 Client::IdleAdd(unsigned flags) noexcept
 {
+	if (IsExpired())
+		return;
+
 	idle_flags |= flags;
 	if (idle_waiting && (idle_flags & idle_subscriptions))
 		IdleNotify();
diff --git a/src/client/Process.cxx b/src/client/Process.cxx
index 8966eca05..bb18d86c6 100644
--- a/src/client/Process.cxx
+++ b/src/client/Process.cxx
@@ -42,7 +42,9 @@ Client::ProcessCommandList(bool list_ok,
 		FormatDebug(client_domain, "process command \"%s\"", cmd);
 		auto ret = command_process(*this, n++, cmd);
 		FormatDebug(client_domain, "command returned %i", int(ret));
-		if (ret != CommandResult::OK)
+		if (IsExpired())
+			return CommandResult::CLOSE;
+		else if (ret != CommandResult::OK)
 			return ret;
 		else if (list_ok)
 			Write("list_OK\n");
@@ -139,6 +141,9 @@ Client::ProcessLine(char *line) noexcept
 				    "[%u] command returned %i",
 				    id, int(ret));
 
+			if (IsExpired())
+				return CommandResult::CLOSE;
+
 			if (ret == CommandResult::OK)
 				command_success(*this);
 
diff --git a/src/client/Read.cxx b/src/client/Read.cxx
index f7fa66036..200a20876 100644
--- a/src/client/Read.cxx
+++ b/src/client/Read.cxx
@@ -69,5 +69,10 @@ Client::OnSocketInput(void *data, size_t length) noexcept
 		return InputResult::CLOSED;
 	}
 
+	if (IsExpired()) {
+		Close();
+		return InputResult::CLOSED;
+	}
+
 	return InputResult::AGAIN;
 }
diff --git a/src/client/Write.cxx b/src/client/Write.cxx
index 264d103e7..14a2b2f39 100644
--- a/src/client/Write.cxx
+++ b/src/client/Write.cxx
@@ -21,6 +21,13 @@
 
 #include <string.h>
 
+bool
+Client::Write(const void *data, size_t length) noexcept
+{
+	/* if the client is going to be closed, do nothing */
+	return !IsExpired() && FullyBufferedSocket::Write(data, length);
+}
+
 bool
 Client::Write(const char *data) noexcept
 {