From 6d72e42a0d84139d3baceb9135e8610efe1ca1c4 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Fri, 15 Apr 2022 10:38:08 +0200 Subject: [PATCH] dnsdist: Prevent leaking the console's socket descriptors I don't see any case where that could happen but better wrap the socket descriptors in FDWrapper objects so that we cannot forget to close them if an exception is raised, for example. --- pdns/dnsdist-console.cc | 139 ++++++++++++++++++++-------------------- 1 file changed, 71 insertions(+), 68 deletions(-) diff --git a/pdns/dnsdist-console.cc b/pdns/dnsdist-console.cc index 61baa69336..2058e804d2 100644 --- a/pdns/dnsdist-console.cc +++ b/pdns/dnsdist-console.cc @@ -58,16 +58,14 @@ static ConcurrentConnectionManager s_connManager(100); class ConsoleConnection { public: - ConsoleConnection(const ComboAddress& client, int fd): d_client(client), d_fd(fd) + ConsoleConnection(const ComboAddress& client, FDWrapper&& fd): d_client(client), d_fd(std::move(fd)) { if (!s_connManager.registerConnection()) { - close(fd); throw std::runtime_error("Too many concurrent console connections"); } } - ConsoleConnection(ConsoleConnection&& rhs): d_client(rhs.d_client), d_fd(rhs.d_fd) + ConsoleConnection(ConsoleConnection&& rhs): d_client(rhs.d_client), d_fd(std::move(rhs.d_fd)) { - rhs.d_fd = -1; } ConsoleConnection(const ConsoleConnection&) = delete; @@ -75,15 +73,14 @@ public: ~ConsoleConnection() { - if (d_fd != -1) { - close(d_fd); + if (d_fd.getHandle() != -1) { s_connManager.releaseConnection(); } } int getFD() const { - return d_fd; + return d_fd.getHandle(); } const ComboAddress& getClient() const @@ -93,7 +90,7 @@ public: private: ComboAddress d_client; - int d_fd{-1}; + FDWrapper d_fd; }; void setConsoleMaximumConcurrentConnections(size_t max) @@ -229,28 +226,30 @@ void doClient(ComboAddress server, const std::string& command) cout<<"Connecting to "< "); rl_bind_key('\t',rl_complete); - if(!sline) + if (!sline) { break; + } string line(sline); - if(!line.empty() && line != lastline) { + if (!line.empty() && line != lastline) { add_history(sline); history << sline < >(withReturn ? ("return "+line) : line); - if(ret) { + if (ret) { if (const auto dsValue = boost::get>(&*ret)) { if (*dsValue) { - response=(*dsValue)->getName()+"\n"; + response = (*dsValue)->getName()+"\n"; } else { - response=""; + response = ""; } } else if (const auto csValue = boost::get(&*ret)) { if (*csValue) { - response=(*csValue)->local.toStringWithPort()+"\n"; + response = (*csValue)->local.toStringWithPort()+"\n"; } else { - response=""; + response = ""; } } else if (const auto strValue = boost::get(&*ret)) { - response=*strValue+"\n"; + response = *strValue+"\n"; } - else if(const auto um = boost::get >(&*ret)) { + else if (const auto um = boost::get >(&*ret)) { using namespace json11; Json::object o; - for(const auto& v : *um) - o[v.first]=v.second; + for(const auto& v : *um) { + o[v.first] = v.second; + } Json out = o; - response=out.dump()+"\n"; + response = out.dump()+"\n"; } } - else - response=g_outputBuffer; - if(!getLuaNoSideEffect()) + else { + response = g_outputBuffer; + } + if (!getLuaNoSideEffect()) { feedConfigDelta(line); + } } - catch(const LuaContext::SyntaxErrorException&) { + catch (const LuaContext::SyntaxErrorException&) { if(withReturn) { withReturn=false; goto retry; @@ -928,23 +931,26 @@ static void controlClientThread(ConsoleConnection&& conn) response = "Command returned an object we can't print: " +std::string(e.what()) + "\n"; // tried to return something we don't understand } - catch(const LuaContext::ExecutionErrorException& e) { - if(!strcmp(e.what(),"invalid key to 'next'")) + catch (const LuaContext::ExecutionErrorException& e) { + if (!strcmp(e.what(),"invalid key to 'next'")) { response = "Error: Parsing function parameters, did you forget parameter name?"; - else + } + else { response = "Error: " + string(e.what()); + } + try { std::rethrow_if_nested(e); - } catch(const std::exception& ne) { + } catch (const std::exception& ne) { // ne is the exception that was thrown from inside the lambda response+= ": " + string(ne.what()); } - catch(const PDNSException& ne) { + catch (const PDNSException& ne) { // ne is the exception that was thrown from inside the lambda response += ": " + string(ne.reason); } } - catch(const LuaContext::SyntaxErrorException& e) { + catch (const LuaContext::SyntaxErrorException& e) { response = "Error: " + string(e.what()) + ": "; } response = sodEncryptSym(response, g_consoleKey, writingNonce); @@ -955,14 +961,14 @@ static void controlClientThread(ConsoleConnection&& conn) infolog("Closed control connection from %s", conn.getClient().toStringWithPort()); } } - catch (const std::exception& e) - { + catch (const std::exception& e) { errlog("Got an exception in client connection from %s: %s", conn.getClient().toStringWithPort(), e.what()); } } void controlThread(int fd, ComboAddress local) { + FDWrapper acceptFD(fd); try { setThreadName("dnsdist/control"); @@ -971,22 +977,21 @@ void controlThread(int fd, ComboAddress local) auto localACL = g_consoleACL.getLocal(); infolog("Accepting control connections on %s", local.toStringWithPort()); - while ((sock = SAccept(fd, client)) >= 0) { + while ((sock = SAccept(acceptFD.getHandle(), client)) >= 0) { + FDWrapper socket(sock); if (!sodIsValidKey(g_consoleKey)) { vinfolog("Control connection from %s dropped because we don't have a valid key configured, please configure one using setKey()", client.toStringWithPort()); - close(sock); continue; } if (!localACL->match(client)) { vinfolog("Control connection from %s dropped because of ACL", client.toStringWithPort()); - close(sock); continue; } try { - ConsoleConnection conn(client, sock); + ConsoleConnection conn(client, std::move(socket)); if (g_logConsoleConnections) { warnlog("Got control connection from %s", client.toStringWithPort()); } @@ -999,9 +1004,7 @@ void controlThread(int fd, ComboAddress local) } } } - catch (const std::exception& e) - { - close(fd); + catch (const std::exception& e) { errlog("Control thread died: %s", e.what()); } } -- 2.47.2