SYNERGY1-1497 Memory leaks in copy/paste and drag and drop functionality (#7175)

* SYNERGY1-1497 Fix memory leak in copy/paste functionality

* SYNERGY1-1497 Memory leaks with function CFStringRefToUTF8String

* SYNERGY1-1497 Fix memory leak in Server::sendDragInfo

* SYNERGY1-1497 Fix memory leak in Server::sendFileToClient

* SYNERGY1-1497 Fix memory leak in Server::sendDragInfoThread

* SYNERGY1-1497 Fix code smells

* SYNERGY1-1497 Fix builds

* SYNERGY1-1497 Fix additional code smells

ChangeLog

@@ -10,6 +10,7 @@ Bug fixes:
 - #7149 Address issues with modifiers and dead keys
 - #7163 Fix compilation issues for FreeBSD
 - #7172 Memory leaks in language sync and TLS functionality
+- #7175 Memory leaks in copy/paste and drag and drop functionality
 
 Github Actions:
 - #7148 Fix unstable build for windows core

src/lib/base/Event.cpp

@@ -43,6 +43,16 @@ Event::Event(Type type, void* target, void* data, Flags flags) :
     // do nothing
 }
 
+Event::Event(Type type, void* target, EventData* dataObject) :
+    m_type(type),
+    m_target(target),
+    m_data(nullptr),
+    m_flags(kNone),
+    m_dataObject(dataObject)
+{
+
+}
+
 Event::Type
 Event::getType() const
 {

src/lib/base/Event.h

@@ -56,13 +56,21 @@ public:
     The \p data must be POD (plain old data) allocated by malloc(),
     which means it cannot have a constructor, destructor or be
     composed of any types that do. For non-POD (normal C++ objects
-    use \c setDataObject().
+    use \c setDataObject() or use appropriate constructor.
     \p target is the intended recipient of the event.
     \p flags is any combination of \c Flags.
     */
     Event(Type type, void* target = NULL, void* data = NULL,
                              Flags flags = kNone);
 
+    //! Create \c Event with non-POD data
+    /*!
+    \p type of the event
+    \p target is the intended recipient of the event.
+    \p dataObject with event data
+    */
+    Event(Type type, void* target, EventData* dataObject);
+
     //! @name manipulators
     //@{
 

src/lib/client/Client.cpp

@@ -72,8 +72,8 @@ Client::Client(
     m_suspended(false),
     m_connectOnResume(false),
     m_events(events),
-    m_sendFileThread(NULL),
-    m_writeToDropDirThread(NULL),
+    m_sendFileThread(nullptr),
+    m_writeToDropDirThread(nullptr),
     m_socket(NULL),
     m_useSecureNetwork(args.m_enableCrypto),
     m_args(args),
@@ -258,9 +258,9 @@ Client::enter(SInt32 xAbs, SInt32 yAbs, UInt32, KeyModifierMask mask, bool)
     m_screen->mouseMove(xAbs, yAbs);
     m_screen->enter(mask);
 
-    if (m_sendFileThread != NULL) {
+    if (m_sendFileThread) {
         StreamChunker::interruptFile();
-        m_sendFileThread = NULL;
+        m_sendFileThread.reset(nullptr);
     }
 }
 
@@ -827,9 +827,8 @@ void
 Client::onFileRecieveCompleted()
 {
     if (isReceivedFileSizeValid()) {
-        m_writeToDropDirThread = new Thread(
-            new TMethodJob<Client>(
-                this, &Client::writeToDropDirThread));
+        auto method = new TMethodJob<Client>(this, &Client::writeToDropDirThread);
+        m_writeToDropDirThread.reset(new Thread(method));
     }
 }
 
@@ -875,14 +874,13 @@ Client::isReceivedFileSizeValid()
 void
 Client::sendFileToServer(const char* filename)
 {
-    if (m_sendFileThread != NULL) {
+    if (m_sendFileThread) {
         StreamChunker::interruptFile();
     }
-    
-    m_sendFileThread = new Thread(
-        new TMethodJob<Client>(
-            this, &Client::sendFileThread,
-            static_cast<void*>(const_cast<char*>(filename))));
+
+    auto data = static_cast<void*>(const_cast<char*>(filename));
+    auto method = new TMethodJob<Client>(this, &Client::sendFileThread, data);
+    m_sendFileThread.reset(new Thread(method));
 }
 
 void
@@ -896,7 +894,7 @@ Client::sendFileThread(void* filename)
         LOG((CLOG_ERR "failed sending file chunks: %s", error.what()));
     }
 
-    m_sendFileThread = NULL;
+    m_sendFileThread.reset(nullptr);
 }
 
 void

src/lib/client/Client.h

@@ -27,6 +27,7 @@
 #include "net/NetworkAddress.h"
 #include "base/EventTypes.h"
 #include "mt/CondVar.h"
+#include <memory>
 
 class EventQueueTimer;
 namespace synergy { class Screen; }
@@ -234,8 +235,9 @@ private:
     String              m_receivedFileData;
     DragFileList        m_dragFileList;
     String              m_dragFileExt;
-    Thread*             m_sendFileThread;
-    Thread*             m_writeToDropDirThread;
+    using AutoThread = std::unique_ptr<Thread>;
+    AutoThread          m_sendFileThread;
+    AutoThread          m_writeToDropDirThread;
     TCPSocket*          m_socket;
     bool                m_useSecureNetwork;
     bool                m_enableClipboard;

src/lib/client/ServerProxy.cpp

@@ -920,7 +920,7 @@ ServerProxy::dragInfoReceived()
 void
 ServerProxy::handleClipboardSendingEvent(const Event& event, void*)
 {
-    ClipboardChunk::send(m_stream, event.getData());
+    ClipboardChunk::send(m_stream, event.getDataObject());
 }
 
 void

src/lib/net/TCPListenSocket.cpp

@@ -152,7 +152,7 @@ TCPListenSocket::serviceListening(ISocketMultiplexerJob* job,
         return NULL;
     }
     if (read) {
-        m_events->addEvent(Event(m_events->forIListenSocket().connecting(), this, NULL));
+        m_events->addEvent(Event(m_events->forIListenSocket().connecting(), this));
         // stop polling on this socket until the client accepts
         return NULL;
     }

src/lib/net/TCPSocket.cpp

@@ -443,7 +443,7 @@ TCPSocket::sendConnectionFailedEvent(const char* msg)
 void
 TCPSocket::sendEvent(Event::Type type)
 {
-    m_events->addEvent(Event(type, getEventTarget(), nullptr));
+    m_events->addEvent(Event(type, getEventTarget()));
 }
 
 void

src/lib/platform/OSXScreen.mm

@@ -614,30 +614,28 @@ void
 OSXScreen::getDropTargetThread(void*)
 {
 #if defined(MAC_OS_X_VERSION_10_7)
-	char* cstr = NULL;
-	
 	// wait for 5 secs for the drop destinaiton string to be filled.
 	UInt32 timeout = ARCH->time() + 5;
-	
+	m_dropTarget.clear();
+
 	while (ARCH->time() < timeout) {
 		CFStringRef cfstr = getCocoaDropTarget();
-		cstr = CFStringRefToUTF8String(cfstr);
+		char* cstr = CFStringRefToUTF8String(cfstr);
 		CFRelease(cfstr);
 		
 		if (cstr != NULL) {
+			LOG((CLOG_DEBUG "drop target: %s", cstr));
+			m_dropTarget = cstr;
+			free(cstr);
 			break;
 		}
 		ARCH->sleep(.1f);
 	}
 	
-	if (cstr != NULL) {
-		LOG((CLOG_DEBUG "drop target: %s", cstr));
-		m_dropTarget = cstr;
-	}
-	else {
+	if (m_dropTarget.empty()) {
 		LOG((CLOG_ERR "failed to get drop target"));
-		m_dropTarget.clear();
 	}
+
 #else
 	LOG((CLOG_WARN "drag drop not supported"));
 #endif
@@ -2069,14 +2067,15 @@ OSXScreen::CFStringRefToUTF8String(CFStringRef aString)
 	}
 	
 	CFIndex length = CFStringGetLength(aString);
-	CFIndex maxSize = CFStringGetMaximumSizeForEncoding(
-		length,
-		kCFStringEncodingUTF8);
+	CFIndex maxSize = CFStringGetMaximumSizeForEncoding(length, kCFStringEncodingUTF8);
 	char* buffer = (char*)malloc(maxSize);
-	if (CFStringGetCString(aString, buffer, maxSize, kCFStringEncodingUTF8)) {
-		return buffer;
+
+	if (!CFStringGetCString(aString, buffer, maxSize, kCFStringEncodingUTF8)) {
+		free(buffer);
+		buffer = NULL;
 	}
-	return NULL;
+
+	return buffer;
 }
 
 void
@@ -2100,21 +2099,20 @@ String&
 OSXScreen::getDraggingFilename()
 {
 	if (m_draggingStarted) {
+		m_draggingFilename.clear();
+
 		CFStringRef dragInfo = getDraggedFileURL();
-		char* info = NULL;
-		info = CFStringRefToUTF8String(dragInfo);
-		if (info == NULL) {
-			m_draggingFilename.clear();
-		}
-		else {
+		char* info = CFStringRefToUTF8String(dragInfo);
+		CFRelease(dragInfo);
+
+		if (info != NULL) {
 			LOG((CLOG_DEBUG "drag info: %s", info));
-			CFRelease(dragInfo);
-			String fileList(info);
-			m_draggingFilename = fileList;
+			m_draggingFilename = info;
+			free(info);
 		}
 
 		// fake a escape key down and up then left mouse button up
-        fakeKeyDown(kKeyEscape, 8192, 1, AppUtil::instance().getCurrentLanguageCode());
+		fakeKeyDown(kKeyEscape, 8192, 1, AppUtil::instance().getCurrentLanguageCode());
 		fakeKeyUp(1);
 		fakeMouseButton(kButtonLeft, false);
 	}

src/lib/server/ClientProxy1_6.cpp

@@ -64,7 +64,7 @@ ClientProxy1_6::setClipboard(ClipboardID id, const IClipboard* clipboard)
 void
 ClientProxy1_6::handleClipboardSendingEvent(const Event& event, void*)
 {
-    ClipboardChunk::send(getStream(), event.getData());
+    ClipboardChunk::send(getStream(), event.getDataObject());
 }
 
 bool

src/lib/server/Server.cpp

@@ -91,13 +91,13 @@ Server::Server(
 	m_lockedToScreen(false),
 	m_screen(screen),
 	m_events(events),
-	m_sendFileThread(NULL),
-	m_writeToDropDirThread(NULL),
+	m_sendFileThread(nullptr),
+	m_writeToDropDirThread(nullptr),
 	m_ignoreFileTransfer(false),
 	m_disableLockToScreen(false),
 	m_enableClipboard(true),
-    m_maximumClipboardSize(INT_MAX),
-	m_sendDragInfoThread(NULL),
+	m_maximumClipboardSize(INT_MAX),
+	m_sendDragInfoThread(nullptr),
 	m_waitDragInfoThread(true),
 	m_args(args)
 {
@@ -1558,7 +1558,7 @@ Server::handleFakeInputEndEvent(const Event&, void*)
 void
 Server::handleFileChunkSendingEvent(const Event& event, void*)
 {
-	onFileChunkSending(event.getData());
+	onFileChunkSending(event.getDataObject());
 }
 
 void
@@ -1862,11 +1862,11 @@ Server::onMouseMovePrimary(SInt32 x, SInt32 y)
 				&& m_screen->isDraggingStarted()
 				&& m_active != newScreen
 				&& m_waitDragInfoThread) {
-				if (m_sendDragInfoThread == NULL) {
-					m_sendDragInfoThread = new Thread(
+				if (!m_sendDragInfoThread) {
+					m_sendDragInfoThread.reset(new Thread(
 						new TMethodJob<Server>(
 							this,
-							&Server::sendDragInfoThread, newScreen));
+							&Server::sendDragInfoThread, newScreen)));
 				}
 
 				return false;
@@ -1909,7 +1909,8 @@ Server::sendDragInfoThread(void* arg)
 		m_dragFileList.clear();
 	}
 	m_waitDragInfoThread = false;
-	m_sendDragInfoThread = NULL;
+	m_sendDragInfoThread.reset(nullptr);
+
 }
 
 void
@@ -1919,15 +1920,10 @@ Server::sendDragInfo(BaseClientProxy* newScreen)
 	UInt32 fileCount = DragInformation::setupDragInfo(m_dragFileList, infoString);
 
 	if (fileCount > 0) {
-		char* info = NULL;
-		size_t size = infoString.size();
-		info = new char[size];
-		memcpy(info, infoString.c_str(), size);
-
 		LOG((CLOG_DEBUG2 "sending drag information to client"));
-		LOG((CLOG_DEBUG3 "dragging file list: %s", info));
-		LOG((CLOG_DEBUG3 "dragging file list string size: %i", size));
-		newScreen->sendDragInfo(fileCount, info, size);
+		LOG((CLOG_DEBUG3 "dragging file list: %s", infoString.c_str()));
+		LOG((CLOG_DEBUG3 "dragging file list string size: %i", infoString.size()));
+		newScreen->sendDragInfo(fileCount, infoString.c_str(), infoString.size());
 	}
 }
 
@@ -2060,9 +2056,9 @@ Server::onMouseMoveSecondary(SInt32 dx, SInt32 dy)
 	} while (false);
 
 	if (jump) {
-		if (m_sendFileThread != NULL) {
+		if (m_sendFileThread) {
 			StreamChunker::interruptFile();
-			m_sendFileThread = NULL;
+			m_sendFileThread.reset(nullptr);
 		}
 
 		SInt32 newX = m_x;
@@ -2126,9 +2122,8 @@ void
 Server::onFileRecieveCompleted()
 {
 	if (isReceivedFileSizeValid()) {
-		m_writeToDropDirThread = new Thread(
-									   new TMethodJob<Server>(
-															   this, &Server::writeToDropDirThread));
+		auto method = new TMethodJob<Server>(this, &Server::writeToDropDirThread);
+		m_writeToDropDirThread.reset(new Thread(method));
 	}
 }
 
@@ -2429,10 +2424,9 @@ Server::sendFileToClient(const char* filename)
 		StreamChunker::interruptFile();
 	}
 
-	m_sendFileThread = new Thread(
-		new TMethodJob<Server>(
-			this, &Server::sendFileThread,
-			static_cast<void*>(const_cast<char*>(filename))));
+	auto data = static_cast<void*>(const_cast<char*>(filename));
+	auto method = new TMethodJob<Server>(this, &Server::sendFileThread, data);
+	m_sendFileThread.reset(new Thread(method));
 }
 
 void
@@ -2447,7 +2441,7 @@ Server::sendFileThread(void* data)
 		LOG((CLOG_ERR "failed sending file chunks, error: %s", error.what()));
 	}
 
-	m_sendFileThread = NULL;
+	m_sendFileThread.reset(nullptr);
 }
 
 void

src/lib/server/Server.h

@@ -33,6 +33,7 @@
 #include "common/stdmap.h"
 #include "common/stdset.h"
 #include "common/stdvector.h"
+#include <memory>
 
 class BaseClientProxy;
 class EventQueueTimer;
@@ -471,19 +472,20 @@ private:
     IEventQueue*        m_events;
 
     // file transfer
+    using AutoThread = std::unique_ptr<Thread>;
     size_t                m_expectedFileSize;
     String                m_receivedFileData;
     DragFileList        m_dragFileList;
     DragFileList        m_fakeDragFileList;
-    Thread*                m_sendFileThread;
-    Thread*                m_writeToDropDirThread;
+    AutoThread          m_sendFileThread;
+    AutoThread          m_writeToDropDirThread;
     String                m_dragFileExt;
     bool                m_ignoreFileTransfer;
     bool                m_disableLockToScreen;
     bool                m_enableClipboard;
-    size_t				m_maximumClipboardSize;
+    size_t              m_maximumClipboardSize;
 
-    Thread*                m_sendDragInfoThread;
+    AutoThread          m_sendDragInfoThread;
     bool                m_waitDragInfoThread;
 
     ClientListener*        m_clientListener;

src/lib/synergy/Chunk.h

@@ -18,13 +18,14 @@
 #pragma once
 
 #include "common/basic_types.h"
+#include <base/EventTypes.h>
 
-class Chunk {
+class Chunk : public EventData {
 public:
     Chunk(size_t size);
     Chunk(Chunk const &) =delete;
     Chunk(Chunk &&) =delete;
-    ~Chunk();
+    ~Chunk() override;
 
     Chunk& operator=(Chunk const &) =delete;
     Chunk& operator=(Chunk &&) =delete;

src/lib/synergy/PacketStreamFilter.cpp

@@ -83,7 +83,7 @@ PacketStreamFilter::read(void* buffer, UInt32 n)
 
     if (m_inputShutdown && m_size == 0) {
         m_events->addEvent(Event(m_events->forIStream().inputShutdown(),
-                        getEventTarget(), NULL));
+                        getEventTarget()));
     }
 
     return n;