Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ipc: remove possible memory corruption due to strcat on provided pointer #4337

Merged
merged 3 commits into from
Apr 9, 2021

Conversation

GovanifY
Copy link
Member

I was using strcat on a getenv pointer which was reused by the system's lib instead of providing a new one.
Because of this behaviour and me mistakenly appending to this pointer, I ended up corrupting the stack if called enough times.
tl;dr: code bad

@GovanifY
Copy link
Member Author

codacy's warning is stupid, getenv returns null terminated strings.

@Warepire
Copy link

I placed my comment in a bad spot, I meant like this (which also made me find that you forgot to free m_socket_name so you have a memory leak):

--- IPC.cpp.orig	2021-03-29 19:35:17.526725932 +0200
+++ IPC.cpp	2021-03-29 19:39:02.478163161 +0200
@@ -85,20 +85,16 @@
 	// fallback in case macOS or other OSes don't implement the XDG base
 	// spec
 	if (runtime_dir == nullptr)
-		m_socket_name = (char*)"/tmp/" IPC_EMULATOR_NAME ".sock";
+		m_socket_name = "/tmp/" IPC_EMULATOR_NAME ".sock";
 	else
 	{
-		m_socket_name = new char[strlen(runtime_dir) + strlen("/" IPC_EMULATOR_NAME ".sock") + 1];
-		strcpy(m_socket_name, runtime_dir);
-		strcat(m_socket_name, "/" IPC_EMULATOR_NAME ".sock");
+		m_socket_name = runtime_dir;
+		m_socket_name += "/" IPC_EMULATOR_NAME ".sock";
 	}
 
 	if (slot != IPC_DEFAULT_SLOT)
 	{
-		// maximum size of .%u
-		char slot_ending[34];
-		sprintf(slot_ending, ".%u", slot);
-		m_socket_name = strcat(m_socket_name, slot_ending);
+		m_socket_name += "." + std::to_string(slot);
 	}
 
 	struct sockaddr_un server;
@@ -110,11 +106,11 @@
 		return;
 	}
 	server.sun_family = AF_UNIX;
-	strcpy(server.sun_path, m_socket_name);
+	strcpy(server.sun_path, m_socket_name.c_str());
 
 	// we unlink the socket so that when releasing this thread the socket gets
 	// freed even if we didn't close correctly the loop
-	unlink(m_socket_name);
+	unlink(m_socket_name.c_str());
 	if (bind(m_sock, (struct sockaddr*)&server, sizeof(struct sockaddr_un)))
 	{
 		Console.WriteLn(Color_Red, "IPC: Error while binding to socket! Shutting down...");
@@ -250,7 +246,7 @@
 #ifdef _WIN32
 	WSACleanup();
 #else
-	unlink(m_socket_name);
+	unlink(m_socket_name.c_str());
 #endif
 	close_portable(m_sock);
 	close_portable(m_msgsock);
--- IPC.h.orig	2021-03-29 19:35:24.000005145 +0200
+++ IPC.h	2021-03-29 19:36:13.569589803 +0200
@@ -25,6 +25,8 @@
 #define IPC_DEFAULT_SLOT 28011
 #define IPC_EMULATOR_NAME "pcsx2"
 
+#include <string>
+
 #include "Utilities/PersistentThread.h"
 #include "System/SysThreads.h"
 #ifdef _WIN32
@@ -48,7 +50,7 @@
 	SOCKET m_msgsock = INVALID_SOCKET;
 #else
 	// absolute path of the socket. Stored in XDG_RUNTIME_DIR, if unset /tmp
-	char* m_socket_name;
+	std::string m_socket_name;
 	int m_sock = 0;
 	// the message socket used in thread's accept().
 	int m_msgsock = 0;

@GovanifY
Copy link
Member Author

@Warepire good catch, thanks!
Indeed the code is more readable with std::string in that case and it does fix the free I forgot about :p

Thanks!

pcsx2/IPC.cpp Outdated Show resolved Hide resolved
@Warepire
Copy link

Looks fine to me now.

@refractionpcsx2 refractionpcsx2 merged commit edeb0d7 into master Apr 9, 2021
@refractionpcsx2 refractionpcsx2 deleted the ipc-getenv branch April 9, 2021 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants