From 19badc4def7d2bee677da3a02e1117f1e051a8cd Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Wed, 11 Jan 2023 17:21:56 +0100 Subject: [PATCH] Make strSplit() simpler and safer Get rid of all the magical re-allocation and shuffling and instead just return a new set of strings that is fully splitted. Will consume a bit more memory, but is a lot safer to use as there is less confusion about ownership of memory. --- common/network/TcpSocket.cxx | 43 +++++++++++++++---------------- common/rfb/LogWriter.cxx | 32 ++++++++++++----------- common/rfb/SSecurityPlain.cxx | 23 ++++++----------- common/rfb/Security.cxx | 8 +++--- common/rfb/util.cxx | 45 +++++++++++++-------------------- common/rfb/util.h | 11 +++----- win/rfb_win32/LaunchProcess.cxx | 4 ++- win/rfb_win32/Win32Util.cxx | 5 ---- win/rfb_win32/Win32Util.h | 2 -- win/vncconfig/Connections.h | 12 ++++----- win/vncconfig/Legacy.cxx | 28 +++++++++----------- 11 files changed, 90 insertions(+), 123 deletions(-) diff --git a/common/network/TcpSocket.cxx b/common/network/TcpSocket.cxx index ec36b33d..4feab0ae 100644 --- a/common/network/TcpSocket.cxx +++ b/common/network/TcpSocket.cxx @@ -504,13 +504,13 @@ void network::createTcpListeners(std::list *listeners, TcpFilter::TcpFilter(const char* spec) { - rfb::CharArray tmp; - tmp.buf = rfb::strDup(spec); - while (tmp.buf) { - rfb::CharArray first; - rfb::strSplit(tmp.buf, ',', &first.buf, &tmp.buf); - if (strlen(first.buf)) - filter.push_back(parsePattern(first.buf)); + std::vector patterns; + + patterns = rfb::strSplit(spec, ','); + + for (size_t i = 0; i < patterns.size(); i++) { + if (!patterns[i].empty()) + filter.push_back(parsePattern(patterns[i].c_str())); } } @@ -603,14 +603,16 @@ TcpFilter::verifyConnection(Socket* s) { TcpFilter::Pattern TcpFilter::parsePattern(const char* p) { TcpFilter::Pattern pattern; - rfb::CharArray addr, pref; - bool prefix_specified; + std::vector parts; int family; initSockets(); - prefix_specified = rfb::strSplit(&p[1], '/', &addr.buf, &pref.buf); - if (addr.buf[0] == '\0') { + parts = rfb::strSplit(&p[1], '/'); + if (parts.size() > 2) + throw Exception("invalid filter specified"); + + if (parts[0].empty()) { // Match any address memset (&pattern.address, 0, sizeof (pattern.address)); pattern.address.u.sa.sa_family = AF_UNSPEC; @@ -618,22 +620,19 @@ TcpFilter::Pattern TcpFilter::parsePattern(const char* p) { } else { struct addrinfo hints; struct addrinfo *ai; - char *p = addr.buf; int result; memset (&hints, 0, sizeof (hints)); hints.ai_family = AF_UNSPEC; hints.ai_flags = AI_NUMERICHOST; // Take out brackets, if present - if (*p == '[') { - size_t len; - p++; - len = strlen (p); - if (len > 0 && p[len - 1] == ']') - p[len - 1] = '\0'; + if (parts[0][0] == '[') { + parts[0].erase(0, 1); + if (!parts[0].empty() && parts[0][parts.size()-1] == ']') + parts[0].erase(parts.size()-1, 1); } - if ((result = getaddrinfo (p, NULL, &hints, &ai)) != 0) { + if ((result = getaddrinfo (parts[0].c_str(), NULL, &hints, &ai)) != 0) { throw GAIException("unable to resolve host by name", result); } @@ -642,14 +641,14 @@ TcpFilter::Pattern TcpFilter::parsePattern(const char* p) { family = pattern.address.u.sa.sa_family; - if (prefix_specified) { + if (parts.size() > 1) { if (family == AF_INET && - rfb::strContains(pref.buf, '.')) { + (parts[1].find('.') != std::string::npos)) { throw Exception("mask no longer supported for filter, " "use prefix instead"); } - pattern.prefixlen = (unsigned int) atoi(pref.buf); + pattern.prefixlen = (unsigned int) atoi(parts[1].c_str()); } else { switch (family) { case AF_INET: diff --git a/common/rfb/LogWriter.cxx b/common/rfb/LogWriter.cxx index a86e9652..398d3872 100644 --- a/common/rfb/LogWriter.cxx +++ b/common/rfb/LogWriter.cxx @@ -76,19 +76,20 @@ LogWriter::getLogWriter(const char* name) { } bool LogWriter::setLogParams(const char* params) { - CharArray logwriterName, loggerName, logLevel; - if (!strSplit(params, ':', &logwriterName.buf, &loggerName.buf) || - !strSplit(loggerName.buf, ':', &loggerName.buf, &logLevel.buf)) { + std::vector parts; + parts = strSplit(params, ':'); + if (parts.size() != 3) { fprintf(stderr,"failed to parse log params:%s\n",params); return false; } - int level = atoi(logLevel.buf); + int level = atoi(parts[2].c_str()); Logger* logger = 0; - if (strcmp("", loggerName.buf) != 0) { - logger = Logger::getLogger(loggerName.buf); - if (!logger) fprintf(stderr,"no logger found! %s\n",loggerName.buf); + if (!parts[1].empty()) { + logger = Logger::getLogger(parts[1].c_str()); + if (!logger) + fprintf(stderr, "no logger found! %s\n", parts[1].c_str()); } - if (strcmp("*", logwriterName.buf) == 0) { + if (parts[0] == "*") { LogWriter* current = log_writers; while (current) { current->setLog(logger); @@ -97,9 +98,9 @@ bool LogWriter::setLogParams(const char* params) { } return true; } else { - LogWriter* logwriter = getLogWriter(logwriterName.buf); + LogWriter* logwriter = getLogWriter(parts[0].c_str()); if (!logwriter) { - fprintf(stderr,"no logwriter found! %s\n",logwriterName.buf); + fprintf(stderr, "no logwriter found! %s\n", parts[0].c_str()); } else { logwriter->setLog(logger); logwriter->setLevel(level); @@ -122,11 +123,12 @@ bool LogParameter::setParam(const char* v) { if (immutable) return true; LogWriter::setLogParams("*::0"); StringParameter::setParam(v); - CharArray logParam; - CharArray params(strDup(getValueStr().c_str())); - while (params.buf) { - strSplit(params.buf, ',', &logParam.buf, ¶ms.buf); - if (strlen(logParam.buf) && !LogWriter::setLogParams(logParam.buf)) + std::vector parts; + parts = strSplit(v, ','); + for (size_t i = 0; i < parts.size(); i++) { + if (parts[i].empty()) + continue; + if (!LogWriter::setLogParams(parts[i].c_str())) return false; } return true; diff --git a/common/rfb/SSecurityPlain.cxx b/common/rfb/SSecurityPlain.cxx index 018ca517..8bef8d8c 100644 --- a/common/rfb/SSecurityPlain.cxx +++ b/common/rfb/SSecurityPlain.cxx @@ -45,22 +45,15 @@ StringParameter PasswordValidator::plainUsers bool PasswordValidator::validUser(const char* username) { - CharArray users(strDup(plainUsers.getValueStr().c_str())); - CharArray user; + std::vector users; - while (users.buf) { - strSplit(users.buf, ',', &user.buf, &users.buf); -#ifdef WIN32 - if (0 == stricmp(user.buf, "*")) - return true; - if (0 == stricmp(user.buf, username)) - return true; -#else - if (!strcmp(user.buf, "*")) - return true; - if (!strcmp(user.buf, username)) - return true; -#endif + users = strSplit(plainUsers, ','); + + for (size_t i = 0; i < users.size(); i++) { + if (users[i] == "*") + return true; + if (users[i] == username) + return true; } return false; } diff --git a/common/rfb/Security.cxx b/common/rfb/Security.cxx index 56d953f0..be07bae3 100644 --- a/common/rfb/Security.cxx +++ b/common/rfb/Security.cxx @@ -206,10 +206,10 @@ const char* rfb::secTypeName(uint32_t num) std::list rfb::parseSecTypes(const char* types_) { std::list result; - CharArray types(strDup(types_)), type; - while (types.buf) { - strSplit(types.buf, ',', &type.buf, &types.buf); - uint32_t typeNum = secTypeNum(type.buf); + std::vector types; + types = strSplit(types_, ','); + for (size_t i = 0; i < types.size(); i++) { + uint32_t typeNum = secTypeNum(types[i].c_str()); if (typeNum != secTypeInvalid) result.push_back(typeNum); } diff --git a/common/rfb/util.cxx b/common/rfb/util.cxx index 30cf0300..6f12ad7d 100644 --- a/common/rfb/util.cxx +++ b/common/rfb/util.cxx @@ -66,35 +66,24 @@ namespace rfb { delete [] s; } - - bool strSplit(const char* src, const char limiter, char** out1, char** out2, bool fromEnd) { - CharArray out1old, out2old; - if (out1) out1old.buf = *out1; - if (out2) out2old.buf = *out2; - int len = strlen(src); - int i=0, increment=1, limit=len; - if (fromEnd) { - i=len-1; increment = -1; limit = -1; - } - while (i!=limit) { - if (src[i] == limiter) { - if (out1) { - *out1 = new char[i+1]; - if (i) memcpy(*out1, src, i); - (*out1)[i] = 0; - } - if (out2) { - *out2 = new char[len-i]; - if (len-i-1) memcpy(*out2, &src[i+1], len-i-1); - (*out2)[len-i-1] = 0; - } - return true; + std::vector strSplit(const char* src, + const char delimiter) + { + std::vector out; + const char *start, *stop; + + start = src; + do { + stop = strchr(start, delimiter); + if (stop == NULL) { + out.push_back(start); + } else { + out.push_back(std::string(start, stop-start)); + start = stop + 1; } - i+=increment; - } - if (out1) *out1 = strDup(src); - if (out2) *out2 = 0; - return false; + } while (stop != NULL); + + return out; } bool strContains(const char* src, char c) { diff --git a/common/rfb/util.h b/common/rfb/util.h index d8d7c653..2540ab3b 100644 --- a/common/rfb/util.h +++ b/common/rfb/util.h @@ -61,14 +61,9 @@ namespace rfb { char* strDup(const char* s); void strFree(char* s); - // Returns true if split successful. Returns false otherwise. - // ALWAYS *copies* first part of string to out1 buffer. - // If limiter not found, leaves out2 alone (null) and just copies to out1. - // If out1 or out2 non-zero, calls strFree and zeroes them. - // If fromEnd is true, splits at end of string rather than beginning. - // Either out1 or out2 may be null, in which case the split will not return - // that part of the string. Obviously, setting both to 0 is not useful... - bool strSplit(const char* src, const char limiter, char** out1, char** out2, bool fromEnd=false); + // Splits a string with the specified delimiter + std::vector strSplit(const char* src, + const char delimiter); // Returns true if src contains c bool strContains(const char* src, char c); diff --git a/win/rfb_win32/LaunchProcess.cxx b/win/rfb_win32/LaunchProcess.cxx index 13d7e950..7d17caf7 100644 --- a/win/rfb_win32/LaunchProcess.cxx +++ b/win/rfb_win32/LaunchProcess.cxx @@ -67,7 +67,9 @@ void LaunchProcess::start(HANDLE userToken, bool createConsole) { CharArray exePath; if (!strContains(exeName.buf, '\\')) { ModuleFileName filename; - CharArray path; splitPath(filename.buf, &path.buf, 0); + CharArray path(strDup(filename.buf)); + if (strContains(path.buf, '\\')) + *strrchr(path.buf, '\\') = '\0'; exePath.buf = new char[strlen(path.buf) + strlen(exeName.buf) + 2]; sprintf(exePath.buf, "%s\\%s", path.buf, exeName.buf); } else { diff --git a/win/rfb_win32/Win32Util.cxx b/win/rfb_win32/Win32Util.cxx index 5f0bdbc7..ad301574 100644 --- a/win/rfb_win32/Win32Util.cxx +++ b/win/rfb_win32/Win32Util.cxx @@ -82,11 +82,6 @@ const char* FileVersionInfo::getVerString(const char* name, DWORD langId) { } -bool splitPath(const char* path, char** dir, char** file) { - return strSplit(path, '\\', dir, file, true); -} - - void centerWindow(HWND handle, HWND parent) { RECT r; MonitorInfo mi(parent ? parent : handle); diff --git a/win/rfb_win32/Win32Util.h b/win/rfb_win32/Win32Util.h index beb22ec7..642e377f 100644 --- a/win/rfb_win32/Win32Util.h +++ b/win/rfb_win32/Win32Util.h @@ -36,8 +36,6 @@ namespace rfb { const char* getVerString(const char* name, DWORD langId = 0x080904b0); }; - bool splitPath(const char* path, char** dir, char** file); - // Center the window to a rectangle, or to a parent window. // Optionally, resize the window to lay within the rect or parent window // If the parent window is NULL then the working area if the window's diff --git a/win/vncconfig/Connections.h b/win/vncconfig/Connections.h index f9b66547..492525f3 100644 --- a/win/vncconfig/Connections.h +++ b/win/vncconfig/Connections.h @@ -100,13 +100,11 @@ namespace rfb { while (SendMessage(listBox, LB_GETCOUNT, 0, 0)) SendMessage(listBox, LB_DELETESTRING, 0, 0); - CharArray tmp; - tmp.buf = strDup(hosts.getValueStr().c_str()); - while (tmp.buf) { - CharArray first; - strSplit(tmp.buf, ',', &first.buf, &tmp.buf); - if (strlen(first.buf)) - SendMessage(listBox, LB_ADDSTRING, 0, (LPARAM)first.buf); + std::vector hostv; + hostv = strSplit(hosts, ','); + for (size_t i = 0; i < hostv.size(); i++) { + if (!hostv[i].empty()) + SendMessage(listBox, LB_ADDSTRING, 0, (LPARAM)hostv[i].c_str()); } onCommand(IDC_RFB_ENABLE, EN_CHANGE); diff --git a/win/vncconfig/Legacy.cxx b/win/vncconfig/Legacy.cxx index c9c5e697..bbd796d0 100644 --- a/win/vncconfig/Legacy.cxx +++ b/win/vncconfig/Legacy.cxx @@ -69,30 +69,26 @@ void LegacyPage::LoadPrefs() // Reformat AuthHosts to Hosts. Wish I'd left the format the same. :( :( :( try { - CharArray tmp(strDup(authHosts.c_str())); - while (tmp.buf) { - - // Split the AuthHosts string into patterns to match - CharArray first; - rfb::strSplit(tmp.buf, ':', &first.buf, &tmp.buf); - if (strlen(first.buf)) { + // Split the AuthHosts string into patterns to match + std::vector patterns; + patterns = rfb::strSplit(authHosts.c_str(), ':'); + for (size_t i = 0; i < patterns.size(); i++) { + if (!patterns[i].empty()) { int bits = 0; char pattern[1+4*4+4]; - pattern[0] = first.buf[0]; + pattern[0] = patterns[i][0]; pattern[1] = 0; // Split the pattern into IP address parts and process - rfb::CharArray address; - address.buf = rfb::strDup(&first.buf[1]); - while (address.buf) { - rfb::CharArray part; - rfb::strSplit(address.buf, '.', &part.buf, &address.buf); + std::vector parts; + parts = rfb::strSplit(&patterns[i][1], '.'); + for (size_t j = 0; j < parts.size(); j++) { if (bits) strcat(pattern, "."); - if (strlen(part.buf) > 3) + if (parts[j].size() > 3) throw rdr::Exception("Invalid IP address part"); - if (strlen(part.buf) > 0) { - strcat(pattern, part.buf); + if (!parts[j].empty()) { + strcat(pattern, parts[j].c_str()); bits += 8; } } -- 2.39.5