From 513aa23a714d2ccba388bca41262610c571dcf43 Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Mon, 27 Nov 2017 23:30:50 +0100 Subject: nxcomp: fix double free Fixes ArcticaProject/nx-libs#569 --- nxcomp/src/ChannelEndPoint.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nxcomp/src/ChannelEndPoint.cpp b/nxcomp/src/ChannelEndPoint.cpp index 4fdf0fad4..1504046c1 100644 --- a/nxcomp/src/ChannelEndPoint.cpp +++ b/nxcomp/src/ChannelEndPoint.cpp @@ -91,6 +91,8 @@ ChannelEndPoint::setSpec(const char *hostName, long port) { int length; if (spec_) free(spec_); + spec_ = NULL; + isUnix_ = false; isTCP_ = false; -- cgit v1.2.3 From 21c742d25361e7ae2dc40f635890f60e6eb0e1d6 Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Mon, 27 Nov 2017 23:31:35 +0100 Subject: nxcomp: simplify free calls free() can handle NULL itself --- nxcomp/src/ChannelEndPoint.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nxcomp/src/ChannelEndPoint.cpp b/nxcomp/src/ChannelEndPoint.cpp index 1504046c1..8cc0c1d41 100644 --- a/nxcomp/src/ChannelEndPoint.cpp +++ b/nxcomp/src/ChannelEndPoint.cpp @@ -58,7 +58,7 @@ ChannelEndPoint::~ChannelEndPoint() void ChannelEndPoint::setSpec(const char *spec) { - if (spec_) free(spec_); + free(spec_); if (spec && strlen(spec)) { @@ -90,7 +90,7 @@ void ChannelEndPoint::setSpec(const char *hostName, long port) { int length; - if (spec_) free(spec_); + free(spec_); spec_ = NULL; isUnix_ = false; @@ -162,7 +162,7 @@ ChannelEndPoint::setDefaultTCPInterface(int publicInterface) { void ChannelEndPoint::setDefaultUnixPath(char *path) { - if (defaultUnixPath_) free(defaultUnixPath_); + free(defaultUnixPath_); if (path && strlen(path)) defaultUnixPath_ = strdup(path); -- cgit v1.2.3 From c4660e109aabc78abda4b80d637312385223537f Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Tue, 28 Nov 2017 20:43:44 +0100 Subject: ChannelEndPoint.cpp: fix two memleaks --- nxcomp/src/ChannelEndPoint.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/nxcomp/src/ChannelEndPoint.cpp b/nxcomp/src/ChannelEndPoint.cpp index 8cc0c1d41..843bf2b35 100644 --- a/nxcomp/src/ChannelEndPoint.cpp +++ b/nxcomp/src/ChannelEndPoint.cpp @@ -54,6 +54,12 @@ ChannelEndPoint::~ChannelEndPoint() if(S_ISSOCK(st.st_mode)) unlink(unixPath); } + free(unixPath); + unixPath = NULL; + free(defaultUnixPath_); + defaultUnixPath_ = NULL; + free(spec_); + spec_ = NULL; } void @@ -90,12 +96,12 @@ void ChannelEndPoint::setSpec(const char *hostName, long port) { int length; - free(spec_); - spec_ = NULL; - isUnix_ = false; isTCP_ = false; + free(spec_); + spec_ = NULL; + if (hostName && strlen(hostName) && port >= 1) { length = snprintf(NULL, 0, "tcp:%s:%ld", hostName, port); @@ -195,7 +201,7 @@ ChannelEndPoint::getPort(long *port) const { bool ChannelEndPoint::getUnixPath(char **unixPath) const { - if (unixPath) *unixPath = 0; + if (unixPath) *unixPath = NULL; long p; char *path = NULL; -- cgit v1.2.3 From 1c09eab703de18430241a11abec0526512d851b9 Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Tue, 28 Nov 2017 21:18:48 +0100 Subject: Loop.cpp: fix two memleaks --- nxcomp/src/Loop.cpp | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index b51d7e7e3..ca9e5ed08 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -4278,15 +4278,20 @@ int ListenConnectionTCP(const char *host, long port, const char *label) int ListenConnection(ChannelEndPoint &endpoint, const char *label) { - char *unixPath, *host; + char *unixPath = NULL, *host = NULL; long port; + int result = -1; if (endpoint.getUnixPath(&unixPath)) { - return ListenConnectionUnix(unixPath, label); + result = ListenConnectionUnix(unixPath, label); } else if (endpoint.getTCPHostAndPort(&host, &port)) { - return ListenConnectionTCP(host, port, label); + result = ListenConnectionTCP(host, port, label); } - return -1; + free(unixPath); + unixPath = NULL; + free(host); + host = NULL; + return result; } static int AcceptConnection(int fd, int domain, const char *label) @@ -6739,10 +6744,20 @@ int ConnectToRemote(ChannelEndPoint &socketAddress) } } + free(unixPath); + unixPath = NULL; + free(hostName); + hostName = NULL; + return pFD; ConnectToRemoteError: + free(unixPath); + unixPath = NULL; + free(hostName); + hostName = NULL; + if (pFD != -1) { close(pFD); -- cgit v1.2.3 From 2814677a7e2b259669708c89ed55b55dc1c46f9c Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Tue, 28 Nov 2017 21:58:07 +0100 Subject: Loop.cpp: fix more memory leaks The thread specific stringstream objects on the stack need to be deleted, not just pop()ed. Fixes ArcticaProject/nx-libs#573 (partially) --- nxcomp/src/Log.h | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/nxcomp/src/Log.h b/nxcomp/src/Log.h index aed929b31..3e355a951 100644 --- a/nxcomp/src/Log.h +++ b/nxcomp/src/Log.h @@ -168,7 +168,13 @@ class NXLog delete pdt->thread_name; while (!pdt->buffer.empty()) { + /* + * get the stringstream object created in new_stack_entry() + * from the stack and delete it after pop() + */ + std::stringstream* tmp = pdt->buffer.top(); (void) pdt->buffer.pop (); + delete tmp; } delete pdt; @@ -240,7 +246,12 @@ class NXLog pthread_sigmask(SIG_BLOCK, &tmp_signal_mask, &orig_signal_mask); if (!pdt->buffer.empty ()) { - const std::string str = pdt->buffer.top()->str(); + /* + * get the stringstream object created in new_stack_entry() + * from the stack and delete it after pop() + */ + std::stringstream *tmp = pdt->buffer.top(); + const std::string str = tmp->str(); if (!str.empty()) { @@ -251,6 +262,9 @@ class NXLog /* Remove from stack. */ pdt->buffer.pop(); + + /* free memory */ + delete tmp; } /* Restore old signal mask. */ -- cgit v1.2.3 From 4dbee3a3f13657577f283bca22b281d7273c19e5 Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Thu, 7 Dec 2017 22:35:59 +0100 Subject: nxcomp: use new macro SAFE_FREE(ptr) Should be used instead of free() calls and will clear the pointer after calling free(). This can prevent double-free or use-after-free errors. --- nxcomp/src/ChannelEndPoint.cpp | 30 ++++++++++++++---------------- nxcomp/src/Loop.cpp | 33 +++++++++++++-------------------- nxcomp/src/Misc.h | 3 +++ nxcomp/src/Pipe.cpp | 6 +++--- 4 files changed, 33 insertions(+), 39 deletions(-) diff --git a/nxcomp/src/ChannelEndPoint.cpp b/nxcomp/src/ChannelEndPoint.cpp index 843bf2b35..fb8549f6a 100644 --- a/nxcomp/src/ChannelEndPoint.cpp +++ b/nxcomp/src/ChannelEndPoint.cpp @@ -37,6 +37,8 @@ #include "NXalert.h" +#include "Misc.h" + ChannelEndPoint::ChannelEndPoint(const char *spec) : defaultTCPPort_(0), defaultTCPInterface_(0), defaultUnixPath_(NULL), spec_(NULL) { @@ -54,17 +56,14 @@ ChannelEndPoint::~ChannelEndPoint() if(S_ISSOCK(st.st_mode)) unlink(unixPath); } - free(unixPath); - unixPath = NULL; - free(defaultUnixPath_); - defaultUnixPath_ = NULL; - free(spec_); - spec_ = NULL; + SAFE_FREE(unixPath); + SAFE_FREE(defaultUnixPath_); + SAFE_FREE(spec_); } void ChannelEndPoint::setSpec(const char *spec) { - free(spec_); + SAFE_FREE(spec_); if (spec && strlen(spec)) { @@ -99,8 +98,7 @@ ChannelEndPoint::setSpec(const char *hostName, long port) { isUnix_ = false; isTCP_ = false; - free(spec_); - spec_ = NULL; + SAFE_FREE(spec_); if (hostName && strlen(hostName) && port >= 1) { @@ -145,9 +143,9 @@ ChannelEndPoint::getSpec(char **socketUri) const { *socketUri = strdup(newSocketUri); } - free(newSocketUri); - free(unixPath); - free(hostName); + SAFE_FREE(newSocketUri); + SAFE_FREE(unixPath); + SAFE_FREE(hostName); if (NULL != *socketUri) return true; @@ -168,7 +166,7 @@ ChannelEndPoint::setDefaultTCPInterface(int publicInterface) { void ChannelEndPoint::setDefaultUnixPath(char *path) { - free(defaultUnixPath_); + SAFE_FREE(defaultUnixPath_); if (path && strlen(path)) defaultUnixPath_ = strdup(path); @@ -337,10 +335,10 @@ ChannelEndPoint &ChannelEndPoint::operator=(const ChannelEndPoint &other) { defaultTCPInterface_ = other.defaultTCPInterface_; old = defaultUnixPath_; defaultUnixPath_ = (other.defaultUnixPath_ ? strdup(other.defaultUnixPath_) : NULL); - free(old); + SAFE_FREE(old); old = spec_; spec_ = (other.spec_ ? strdup(other.spec_) : NULL); - free(old); + SAFE_FREE(old); isUnix_ = getUnixPath(); isTCP_ = getTCPHostAndPort(); return *this; @@ -352,7 +350,7 @@ std::ostream& operator<<(std::ostream& os, const ChannelEndPoint& endPoint) { if (endPoint.getSpec(&endPointSpec)) { os << endPointSpec; - free(endPointSpec); + SAFE_FREE(endPointSpec); } else os << "(invalid)"; diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index ca9e5ed08..baad17699 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -3187,8 +3187,7 @@ int SetupProxyConnection() nxinfo << "Loop: listenSocket is "<< ( listenSocket.enabled() ? "enabled" : "disabled") << ". " << "The socket URI is '"<< ( socketUri != NULL ? socketUri : "") << "'.\n" << std::flush; - free(socketUri); - socketUri = NULL; + SAFE_FREE(socketUri); if (WE_INITIATE_CONNECTION) { @@ -3196,7 +3195,7 @@ int SetupProxyConnection() { nxinfo << "Loop: Going to connect to '" << socketUri << "'.\n" << std::flush; - free(socketUri); + SAFE_FREE(socketUri); proxyFD = ConnectToRemote(connectSocket); @@ -3219,7 +3218,7 @@ int SetupProxyConnection() { nxinfo << "Loop: Going to wait for connection at '" << socketUri << "'.\n" << std::flush; - free(socketUri); + SAFE_FREE(socketUri); proxyFD = WaitForRemote(listenSocket); @@ -4287,10 +4286,8 @@ int ListenConnection(ChannelEndPoint &endpoint, const char *label) else if (endpoint.getTCPHostAndPort(&host, &port)) { result = ListenConnectionTCP(host, port, label); } - free(unixPath); - unixPath = NULL; - free(host); - host = NULL; + SAFE_FREE(unixPath); + SAFE_FREE(host); return result; } @@ -6222,7 +6219,7 @@ int WaitForRemote(ChannelEndPoint &socketAddress) cerr << "Info" << ": Waiting for connection from " << hostLabel << " on socket '" << socketUri << "'.\n"; - free(socketUri); + SAFE_FREE(socketUri); // // How many times to loop waiting for connections @@ -6311,7 +6308,7 @@ int WaitForRemote(ChannelEndPoint &socketAddress) cerr << "Info" << ": Accepted connection from this host on Unix file socket '" << unixPath << "'.\n"; - free(unixPath); + SAFE_FREE(unixPath); break; } @@ -6744,19 +6741,15 @@ int ConnectToRemote(ChannelEndPoint &socketAddress) } } - free(unixPath); - unixPath = NULL; - free(hostName); - hostName = NULL; + SAFE_FREE(unixPath); + SAFE_FREE(hostName); return pFD; ConnectToRemoteError: - free(unixPath); - unixPath = NULL; - free(hostName); - hostName = NULL; + SAFE_FREE(unixPath); + SAFE_FREE(hostName); if (pFD != -1) { @@ -7953,7 +7946,7 @@ int ParseEnvironmentOptions(const char *env, int force) cerr << "Error" << ": Refusing 'listen' parameter with 'connect' being '" << socketUri << "'.\n"; - free(socketUri); + SAFE_FREE(socketUri); return -1; } @@ -7981,7 +7974,7 @@ int ParseEnvironmentOptions(const char *env, int force) cerr << "Error" << ": Refusing 'accept' parameter with 'connect' being '" << socketUri << "'.\n"; - free(socketUri); + SAFE_FREE(socketUri); return -1; } diff --git a/nxcomp/src/Misc.h b/nxcomp/src/Misc.h index 997630137..7808c34c2 100644 --- a/nxcomp/src/Misc.h +++ b/nxcomp/src/Misc.h @@ -54,6 +54,9 @@ using namespace std; #define EGET() (errno) #define ESTR() strerror(errno) +// a free() macro that clears the ptr after free +#define SAFE_FREE(ptr) do { free(ptr); ptr = NULL; } while (0) + // // TCP port offset applied to NX port specification. // diff --git a/nxcomp/src/Pipe.cpp b/nxcomp/src/Pipe.cpp index 4fa149412..228c556ae 100644 --- a/nxcomp/src/Pipe.cpp +++ b/nxcomp/src/Pipe.cpp @@ -203,7 +203,7 @@ FILE *Popen(char * const parameters[], const char *type) if (pipe(pdes) < 0) { - free(cur); + SAFE_FREE(cur); return NULL; } @@ -237,7 +237,7 @@ FILE *Popen(char * const parameters[], const char *type) close(pdes[0]); close(pdes[1]); - free(cur); + SAFE_FREE(cur); return NULL; } @@ -420,7 +420,7 @@ int Pclose(FILE *iop) last -> next = cur -> next; } - free(cur); + SAFE_FREE(cur); // // Child has finished and we called the -- cgit v1.2.3