From 604e09686f1f7227cf61e67754d94006c90d29c4 Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 12:12:20 +0100 Subject: Loop.cpp: Fix memset (size was 0) (partially) fixes ArcticaProject/nx-libs#612 --- nxcomp/src/Loop.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index d458d65a3..a4c7a95e7 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -3936,7 +3936,7 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, unixSocketName[0] = '\0'; sockaddr_un *xServerAddrABSTRACT = new sockaddr_un; - memset(xServerAddrABSTRACT, 0, addr_length); + memset(xServerAddrABSTRACT, 0, sizeof(struct sockaddr_un)); xServerAddrABSTRACT -> sun_family = AF_UNIX; memcpy(xServerAddrABSTRACT -> sun_path, unixSocketName, len+1); addr_length = len +3; -- cgit v1.2.3 From 4107159e218c5d641306acf34abcb7596f63f7a5 Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 12:13:43 +0100 Subject: Loop.cpp: free display before leaving SetupDisplaySocket() fixes a memory leak --- nxcomp/src/Loop.cpp | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index a4c7a95e7..ce1f92a68 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -3877,6 +3877,8 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, cerr << "Error" << ": Invalid display '" << display << "'.\n"; + delete [] display; + HandleCleanup(); } @@ -3949,6 +3951,7 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, close(testSocketFD); addr = (sockaddr *) xServerAddrABSTRACT; + delete [] display; return; } else { @@ -4000,6 +4003,7 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, cerr << "Error" << ": Error " << EGET() << " '" << ESTR() << "' checking '" << unixSocketDir << "'.\n"; + delete [] display; HandleCleanup(); } @@ -4048,6 +4052,7 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, cerr << "Error" << ": Unknown display host '" << display << "'.\n"; + delete [] display; HandleCleanup(); } -- cgit v1.2.3 From b593edeb2b2d62434ca19418974227cc4ecaefa1 Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 12:14:38 +0100 Subject: Loop.cpp: always close testSocketFD was missing in the non-abstract code path (partially) fixes ArcticaProject/nx-libs#612 --- nxcomp/src/Loop.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index ce1f92a68..cc0efa361 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -3944,12 +3944,13 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, addr_length = len +3; int ret = connect(testSocketFD, (struct sockaddr *) xServerAddrABSTRACT, addr_length); + close(testSocketFD); + if (ret == 0) { cerr << "Info" << ": Using abstract X11 socket in kernel namespace " << "for accessing DISPLAY=:" << xPort << ".\n"; - close(testSocketFD); addr = (sockaddr *) xServerAddrABSTRACT; delete [] display; return; -- cgit v1.2.3 From f1905c86470e5b76d72af12c9c77ecc719096b3f Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 12:15:47 +0100 Subject: Loop.cpp: delete structs when no longer required Fix another memleak (partially) fixes ArcticaProject/nx-libs#612 --- nxcomp/src/Loop.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index cc0efa361..5b6c2b4bc 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -3952,6 +3952,7 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, << "for accessing DISPLAY=:" << xPort << ".\n"; addr = (sockaddr *) xServerAddrABSTRACT; + delete xServerAddrUNIX; delete [] display; return; @@ -3960,6 +3961,7 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, cerr << "Info" << ": Falling back to file system X11 socket " << "for accessing DISPLAY=:" << xPort << ".\n"; + delete xServerAddrABSTRACT; #endif struct stat statInfo; -- cgit v1.2.3 From c48748ba0929a488437d85732032aef78528093b Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 12:18:28 +0100 Subject: ChannelEndPoint.cpp: re-scope/improve getSpec --- nxcomp/src/Loop.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index 5b6c2b4bc..fa8ba9a1e 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -6222,6 +6222,8 @@ int WaitForRemote(ChannelEndPoint &socketAddress) pFD = ListenConnection(socketAddress, "NX"); + SAFE_FREE(socketUri); + socketAddress.getSpec(&socketUri); nxinfo << "Loop: Waiting for connection from " << hostLabel << " on socket '" << socketUri -- cgit v1.2.3 From c31c54c1b65080b7e68942e8b50dc88b5ce1279b Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 12:54:30 +0100 Subject: Loop.cpp: delete passed object prior to overwriting it --- nxcomp/src/Loop.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index fa8ba9a1e..e84896e36 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -3141,6 +3141,9 @@ int InitBeforeNegotiation() // Get ready to open the local display. // + delete xServerAddr; + xServerAddr = NULL; + SetupDisplaySocket(xServerAddrFamily, xServerAddr, xServerAddrLength); } @@ -3780,13 +3783,13 @@ void SetupUnixSocket() // The following is a dumb copy-paste. The // nxcompsh library should offer a better // implementation. +// addr is assumed to have been freed outside // void SetupDisplaySocket(int &addr_family, sockaddr *&addr, unsigned int &addr_length) { addr_family = AF_INET; - addr = NULL; addr_length = 0; char *display; -- cgit v1.2.3 From 3066195d79f5f24f99483d29e2489a91c9251d73 Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 13:09:25 +0100 Subject: Loop.cpp: some reformatting/simplification/FIXMEs (partially) fixes ArcticaProject/nx-libs#612 --- nxcomp/src/Loop.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index e84896e36..e922b55a5 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -3861,7 +3861,8 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, #ifdef __APPLE__ - if ((strncasecmp(display, "/tmp/launch", 11) == 0) || (strncasecmp(display, "/private/tmp/com.apple.launchd", 30) == 0)) + if ((strncasecmp(display, "/tmp/launch", 11) == 0) || + (strncasecmp(display, "/private/tmp/com.apple.launchd", 30) == 0)) { nxinfo << "Loop: Using launchd service on socket '" << display << "'.\n" << std::flush; @@ -3934,19 +3935,24 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, // fall back to Unix domain socket file. #ifdef __linux__ - int testSocketFD; - testSocketFD = socket(addr_family, SOCK_STREAM, PF_UNSPEC); + int testSocketFD = socket(addr_family, SOCK_STREAM, PF_UNSPEC); + // this name cannot be changed as it is defined this way by the + // local X server int len = sprintf(unixSocketName + 1, "/tmp/.X11-unix/X%d", xPort); unixSocketName[0] = '\0'; sockaddr_un *xServerAddrABSTRACT = new sockaddr_un; memset(xServerAddrABSTRACT, 0, sizeof(struct sockaddr_un)); xServerAddrABSTRACT -> sun_family = AF_UNIX; + // FIXME: ensure sun_path can hold len+1 bytes! memcpy(xServerAddrABSTRACT -> sun_path, unixSocketName, len+1); - addr_length = len +3; + // FIXME: comment why + 3? + addr_length = len + 3; - int ret = connect(testSocketFD, (struct sockaddr *) xServerAddrABSTRACT, addr_length); + int ret = connect(testSocketFD, + (struct sockaddr *) xServerAddrABSTRACT, + addr_length); close(testSocketFD); if (ret == 0) { -- cgit v1.2.3 From 14a5589186c7cf0ececadedecd3db3125466d22a Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 13:16:35 +0100 Subject: Loop.cpp: improve/fix usage of s(n)printf --- nxcomp/src/Loop.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index e922b55a5..c6dc3fefe 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -3939,7 +3939,8 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, // this name cannot be changed as it is defined this way by the // local X server - int len = sprintf(unixSocketName + 1, "/tmp/.X11-unix/X%d", xPort); + int len = snprintf(unixSocketName + 1, DEFAULT_STRING_LENGTH - 1, + "/tmp/.X11-unix/X%d", xPort); unixSocketName[0] = '\0'; sockaddr_un *xServerAddrABSTRACT = new sockaddr_un; @@ -3977,7 +3978,7 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, char unixSocketDir[DEFAULT_STRING_LENGTH]; - snprintf(unixSocketDir, DEFAULT_STRING_LENGTH - 1, "/tmp/.X11-unix"); + snprintf(unixSocketDir, DEFAULT_STRING_LENGTH, "/tmp/.X11-unix"); #ifdef __APPLE__ @@ -3990,7 +3991,7 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, *slash = '\0'; } - snprintf(unixSocketDir, DEFAULT_STRING_LENGTH - 1, "%s", display); + snprintf(unixSocketDir, DEFAULT_STRING_LENGTH, "%s", display); } #endif @@ -4019,7 +4020,8 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, HandleCleanup(); } - sprintf(unixSocketName, "%s/X%d", unixSocketDir, xPort); + snprintf(unixSocketName, DEFAULT_STRING_LENGTH, "%s/X%d", + unixSocketDir, xPort); #ifdef __APPLE__ -- cgit v1.2.3 From 6d8fe661eb792ff9527d78073e361bb67c3b0ede Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 14:43:22 +0100 Subject: Loop.cpp: create xServerAddrUNIX only if required --- nxcomp/src/Loop.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index c6dc3fefe..1bf3a6198 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -3912,10 +3912,7 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, nxinfo << "Loop: Using real X server on UNIX domain socket.\n" << std::flush; - sockaddr_un *xServerAddrUNIX = new sockaddr_un; - addr_family = AF_UNIX; - xServerAddrUNIX -> sun_family = AF_UNIX; // // The scope of this function is to fill either the sockaddr_un @@ -3962,7 +3959,6 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, << "for accessing DISPLAY=:" << xPort << ".\n"; addr = (sockaddr *) xServerAddrABSTRACT; - delete xServerAddrUNIX; delete [] display; return; @@ -4035,6 +4031,9 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, nxinfo << "Loop: Assuming X socket name '" << unixSocketName << "'.\n" << std::flush; + sockaddr_un *xServerAddrUNIX = new sockaddr_un; + + xServerAddrUNIX -> sun_family = AF_UNIX; strcpy(xServerAddrUNIX -> sun_path, unixSocketName); addr = (sockaddr *) xServerAddrUNIX; -- cgit v1.2.3 From 2367fc714843fa0c19778e5a8e100c5f116255ee Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 14:43:50 +0100 Subject: Loop.cpp: drop ugly ifdef indentation --- nxcomp/src/Loop.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index 1bf3a6198..d094bb729 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -3962,13 +3962,14 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, delete [] display; return; - } else { + } - cerr << "Info" << ": Falling back to file system X11 socket " - << "for accessing DISPLAY=:" << xPort << ".\n"; + cerr << "Info" << ": Falling back to file system X11 socket " + << "for accessing DISPLAY=:" << xPort << ".\n"; - delete xServerAddrABSTRACT; - #endif + delete xServerAddrABSTRACT; + +#endif struct stat statInfo; @@ -4039,10 +4040,6 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, addr = (sockaddr *) xServerAddrUNIX; addr_length = sizeof(sockaddr_un); - #ifdef __linux__ - - } - #endif } else { -- cgit v1.2.3 From 6198e0376f9ce8130af3294fa284659f0055610d Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 14:53:58 +0100 Subject: nxcomp: implement correct length handling for unix socket structs (partially) fixes ArcticaProject/nx-libs#612 --- nxcomp/src/Loop.cpp | 67 ++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 51 insertions(+), 16 deletions(-) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index d094bb729..3ee094efb 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -55,6 +55,8 @@ #include "Misc.h" +#include + #ifdef __sun #include #endif @@ -3590,19 +3592,14 @@ int SetupAuthInstance() launchdAddrUnix.sun_family = AF_UNIX; - #ifdef __linux__ - const int launchdAddrNameLength = 108; - #else - /* POSIX/SUS does not specify a length. - * BSD derivatives generally support 104 bytes, other systems may be more constrained. - * If you happen to run into such systems, extend this section with the appropriate limit. - */ - const int launchdAddrNameLength = 104; - #endif + // determine the maximum number of characters that fit into struct + // sockaddr_un's sun_path member + std::size_t launchdAddrNameLength = + sizeof(struct sockaddr_un) - offsetof(struct sockaddr_un, sun_path); int success = -1; - strncpy(launchdAddrUnix.sun_path, displayHost, launchdAddrNameLength); + snprintf(launchdAddrUnix.sun_path, launchdAddrNameLength, "%s", displayHost); *(launchdAddrUnix.sun_path + launchdAddrNameLength - 1) = '\0'; @@ -3909,6 +3906,11 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, // UNIX domain port. // + // determine the maximum number of characters that fit into struct + // sockaddr_un's sun_path member + std::size_t maxlen_un = + sizeof(struct sockaddr_un) - offsetof(struct sockaddr_un, sun_path); + nxinfo << "Loop: Using real X server on UNIX domain socket.\n" << std::flush; @@ -3943,10 +3945,28 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, sockaddr_un *xServerAddrABSTRACT = new sockaddr_un; memset(xServerAddrABSTRACT, 0, sizeof(struct sockaddr_un)); xServerAddrABSTRACT -> sun_family = AF_UNIX; - // FIXME: ensure sun_path can hold len+1 bytes! - memcpy(xServerAddrABSTRACT -> sun_path, unixSocketName, len+1); - // FIXME: comment why + 3? - addr_length = len + 3; + + if (maxlen_un < (unsigned int)len + 1) + { + nxfatal << "Loop: PANIC! Abstract socket name '" << unixSocketName + 1 + << "' is too long!" << std::flush; + + delete [] display; + delete xServerAddrABSTRACT; + + HandleCleanup(); + } + + // copy including the leading '\0' + memcpy(xServerAddrABSTRACT -> sun_path, unixSocketName, len + 1); + + // man 7 unix: + // "an abstract socket address is distinguished (from a + // pathname socket) by the fact that sun_path[0] is a null byte + // ('\0'). The socket's address in this namespace is given by the + // additional bytes in sun_path that are covered by the specified + // length of the address structure." + addr_length = offsetof(struct sockaddr_un, sun_path) + len + 1; int ret = connect(testSocketFD, (struct sockaddr *) xServerAddrABSTRACT, @@ -4032,8 +4052,17 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, nxinfo << "Loop: Assuming X socket name '" << unixSocketName << "'.\n" << std::flush; - sockaddr_un *xServerAddrUNIX = new sockaddr_un; + if (maxlen_un < strlen(unixSocketName) + 1) + { + nxfatal << "Loop: PANIC! Socket name '" << unixSocketName + << "' is too long!" << std::flush; + delete [] display; + + HandleCleanup(); + } + + sockaddr_un *xServerAddrUNIX = new sockaddr_un; xServerAddrUNIX -> sun_family = AF_UNIX; strcpy(xServerAddrUNIX -> sun_path, unixSocketName); @@ -6539,12 +6568,18 @@ int PrepareProxyConnectionUnix(char** path, int* timeout, int* proxyFileDescript /* FIXME: Add socket file existence and permission checks */ + *proxyFileDescriptor = -1; *reason = -1; + // determine the maximum number of characters that fit into struct + // sockaddr_un's sun_path member + const std::size_t sockpathlen = + sizeof(struct sockaddr_un) - offsetof(struct sockaddr_un, sun_path); + sockaddr_un addr; addr.sun_family = AF_UNIX; - strncpy(addr.sun_path, *path, 108 - 1); + snprintf(addr.sun_path, sockpathlen, "%s", *path); *proxyFileDescriptor = socket(AF_UNIX, SOCK_STREAM, PF_UNSPEC); *reason = EGET(); -- cgit v1.2.3 From eae64c4a4282eb2b511ba11c6db51d00a3b49833 Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 17:06:43 +0100 Subject: ChannelEndPoint.cpp: fix another memleak ==7689== 50 bytes in 5 blocks are definitely lost in loss record 1 of 2 ==7689== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==7689== by 0x54074D9: strndup (strndup.c:43) ==7689== by 0x4E7D803: ChannelEndPoint::getTCPHostAndPort(char**, long*) const (ChannelEndPoint.cpp:309) ==7689== by 0x4EC9D93: ConnectToRemote(ChannelEndPoint&) [clone .constprop.144] (Loop.cpp:6660) ==7689== by 0x4ECB94E: SetupProxyConnection() (Loop.cpp:3204) ==7689== by 0x4ECE824: handleNegotiationInLoop(int&, fd_set&, fd_set&, timeval&) [clone .isra.129] (Loop.cpp:14312) ==7689== by 0x4ED0F8A: NXTransPrepare (Loop.cpp:2575) ==7689== by 0x4ED1C35: NXTransContinue (Loop.cpp:1609) ==7689== by 0x4ED1D7B: WaitCleanup() (Loop.cpp:4440) ==7689== by 0x4ED2343: NXTransProxy (Loop.cpp:1234) ==7689== by 0x400B2A: main (Main.c:111) --- nxcomp/src/Loop.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index 3ee094efb..ab7ea5a0f 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -6660,6 +6660,8 @@ int ConnectToRemote(ChannelEndPoint &socketAddress) << " in process with pid '" << getpid() << "'.\n" << std::flush; + SAFE_FREE(hostName); + if (socketAddress.getUnixPath(&unixPath)) result = PrepareProxyConnectionUnix(&unixPath, &connectTimeout, &pFD, &reason); else if (socketAddress.getTCPHostAndPort(&hostName, &portNum)) -- cgit v1.2.3 From ce293647d5a63726c05260ca0e0f65a50e604ebb Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 19:16:15 +0100 Subject: ChannelEndPoint.cpp: fix possible memleak in getUnixPath() --- nxcomp/src/Loop.cpp | 1 + 1 file changed, 1 insertion(+) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index ab7ea5a0f..1e770380f 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -6661,6 +6661,7 @@ int ConnectToRemote(ChannelEndPoint &socketAddress) << "'.\n" << std::flush; SAFE_FREE(hostName); + SAFE_FREE(unixPath); if (socketAddress.getUnixPath(&unixPath)) result = PrepareProxyConnectionUnix(&unixPath, &connectTimeout, &pFD, &reason); -- cgit v1.2.3 From 9e8bd2e1b6029ef04dec424fefcdf8842a0daf0f Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Wed, 27 Dec 2017 21:13:39 +0100 Subject: Loop.cpp: fix memleak happening with unknown tcp host ==28424== 6 bytes in 1 blocks are definitely lost in loss record 1 of 7 ==28424== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==28424== by 0x541D4D9: strndup (strndup.c:43) ==28424== by 0x4E8AD4B: ChannelEndPoint::getTCPHostAndPort(char**, long*) const (ChannelEndPoint.cpp:311) ==28424== by 0x4EBE9CC: ConnectToRemote(ChannelEndPoint&) (Loop.cpp:6656) ==28424== by 0x4EB0A4C: SetupProxyConnection() (Loop.cpp:3205) ==28424== by 0x4EDC81A: handleNegotiationInLoop(int&, fd_set&, fd_set&, timeval&) (Loop.cpp:14308) ==28424== by 0x4EAE40F: NXTransPrepare (Loop.cpp:2576) ==28424== by 0x4EAA801: NXTransContinue (Loop.cpp:1610) ==28424== by 0x4EB50C9: WaitCleanup() (Loop.cpp:4436) ==28424== by 0x4EA9507: NXTransProxy (Loop.cpp:1235) ==28424== by 0x400B2A: main (Main.c:111) --- nxcomp/src/Loop.cpp | 1 + 1 file changed, 1 insertion(+) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index 1e770380f..b0b778a14 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -6482,6 +6482,7 @@ int PrepareProxyConnectionTCP(char** hostName, long int* portNum, int* timeout, cerr << "Error" << ": Unknown remote host '" << *hostName << "'.\n"; + SAFE_FREE(*hostName); HandleCleanup(); } -- cgit v1.2.3 From 6e98e35cf24016c9789be26d33d918f6e0e3c9a1 Mon Sep 17 00:00:00 2001 From: Ulrich Sibiller Date: Thu, 28 Dec 2017 11:19:44 +0100 Subject: nxcomp: drop strncpy in favour of snprintf with very few exceptions which require careful thinking ;-) --- nxcomp/src/Loop.cpp | 44 ++++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 24 deletions(-) (limited to 'nxcomp/src/Loop.cpp') diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index b0b778a14..6b903789b 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -4044,7 +4044,7 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, if (useLaunchdSocket == 1) { - strncpy(unixSocketName, displayHost, DEFAULT_STRING_LENGTH - 1); + snprintf(unixSocketName, DEFAULT_STRING_LENGTH, "%s", displayHost); } #endif @@ -7923,11 +7923,11 @@ int ParseEnvironmentOptions(const char *env, int force) if (strcasecmp(name, "options") == 0) { - strncpy(fileOptions, value, DEFAULT_STRING_LENGTH - 1); + snprintf(fileOptions, DEFAULT_STRING_LENGTH, "%s", value); } else if (strcasecmp(name, "display") == 0) { - strncpy(displayHost, value, DEFAULT_STRING_LENGTH - 1); + snprintf(displayHost, DEFAULT_STRING_LENGTH, "%s", value); } else if (strcasecmp(name, "link") == 0) { @@ -7983,7 +7983,7 @@ int ParseEnvironmentOptions(const char *env, int force) } else { - strncpy(sessionType, value, DEFAULT_STRING_LENGTH - 1); + snprintf(sessionType, DEFAULT_STRING_LENGTH, "%s", value); } } } @@ -8036,7 +8036,7 @@ int ParseEnvironmentOptions(const char *env, int force) return -1; } - strncpy(acceptHost, value, DEFAULT_STRING_LENGTH - 1); + snprintf(acceptHost, DEFAULT_STRING_LENGTH, "%s", value); } else if (strcasecmp(name, "connect") == 0) { @@ -8074,7 +8074,7 @@ int ParseEnvironmentOptions(const char *env, int force) } else if (strcasecmp(name, "session") == 0) { - strncpy(sessionFileName, value, DEFAULT_STRING_LENGTH - 1); + snprintf(sessionFileName, DEFAULT_STRING_LENGTH, "%s", value); } else if (strcasecmp(name, "errors") == 0) { @@ -8085,27 +8085,27 @@ int ParseEnvironmentOptions(const char *env, int force) // the same name. // - strncpy(errorsFileName, value, DEFAULT_STRING_LENGTH - 1); + snprintf(errorsFileName, DEFAULT_STRING_LENGTH, "%s", value); } else if (strcasecmp(name, "root") == 0) { - strncpy(rootDir, value, DEFAULT_STRING_LENGTH - 1); + snprintf(rootDir, DEFAULT_STRING_LENGTH, "%s", value); } else if (strcasecmp(name, "id") == 0) { - strncpy(sessionId, value, DEFAULT_STRING_LENGTH - 1); + snprintf(sessionId, DEFAULT_STRING_LENGTH, "%s", value); } else if (strcasecmp(name, "stats") == 0) { control -> EnableStatistics = 1; - strncpy(statsFileName, value, DEFAULT_STRING_LENGTH - 1); + snprintf(statsFileName, DEFAULT_STRING_LENGTH, "%s", value); } else if (strcasecmp(name, "cookie") == 0) { LowercaseArg("local", name, value); - strncpy(authCookie, value, DEFAULT_STRING_LENGTH - 1); + snprintf(authCookie, DEFAULT_STRING_LENGTH, "%s", value); } else if (strcasecmp(name, "nodelay") == 0) { @@ -8334,7 +8334,7 @@ int ParseEnvironmentOptions(const char *env, int force) } else if (strcasecmp(name, "font") == 0) { - strncpy(fontPort, value, DEFAULT_STRING_LENGTH - 1); + snprintf(fontPort, DEFAULT_STRING_LENGTH, "%s", value); } else if (strcasecmp(name, "slave") == 0) { @@ -8439,7 +8439,7 @@ int ParseEnvironmentOptions(const char *env, int force) } else if (strcasecmp(name, "product") == 0) { - strncpy(productName, value, DEFAULT_STRING_LENGTH - 1); + snprintf(productName, DEFAULT_STRING_LENGTH, "%s", value); } else if (strcasecmp(name, "rootless") == 0 || strcasecmp(name, "geometry") == 0 || @@ -8529,7 +8529,7 @@ int ParseEnvironmentOptions(const char *env, int force) if (*optionsFileName == '\0') { - strncpy(optionsFileName, value, DEFAULT_STRING_LENGTH - 1); + snprintf(optionsFileName, DEFAULT_STRING_LENGTH, "%s", value); nxinfo << "Loop: Assuming name of options file '" << optionsFileName << "'.\n" @@ -9249,7 +9249,7 @@ int ParseRemoteOptions(char *opts) } else { - strncpy(sessionType, value, DEFAULT_STRING_LENGTH - 1); + snprintf(sessionType, DEFAULT_STRING_LENGTH, "%s", value); } } @@ -12719,6 +12719,7 @@ int ParseHostOption(const char *opt, char *host, long &port) char newHost[DEFAULT_STRING_LENGTH] = { 0 }; + // opt cannot be longer than DEFAULT_STRING_LENGTH, this is checked above strncpy(newHost, opt, strlen(opt) - strlen(separator)); *(newHost + strlen(opt) - strlen(separator)) = '\0'; @@ -13491,10 +13492,8 @@ int ParseArg(const char *type, const char *name, const char *value) char *string = new char[strlen(value)]; - strncpy(string, value, strlen(value) - 1); - - *(string + (strlen(value) - 1)) = '\0'; - + // copy value but cut off the last character + snprintf(string, strlen(value), "%s", value); nxinfo << "Loop: Parsing integer option '" << name << "' from string '" << string << "' with base set to "; @@ -13512,18 +13511,15 @@ int ParseArg(const char *type, const char *name, const char *value) nxinfo_append << ".\n" << std::flush; - double result = atof(string) * base; + delete [] string; + if (result < 0 || result > (((unsigned) -1) >> 1)) { - delete [] string; - return -1; } - delete [] string; - nxinfo << "Loop: Integer option parsed to '" << (int) result << "'.\n" << std::flush; -- cgit v1.2.3