Skip to content

Commit 14dc0a4

Browse files
cataphractcataphract
authored
Merge pull request #3526 from DataDog/anilm3/appsec-fd-fixes
[AAP] Minor fixes and improvements to file descriptor reclamation
2 parents 01a8843 + f49dd68 commit 14dc0a4

7 files changed

Lines changed: 48 additions & 31 deletions

File tree

‎appsec/.clang-tidy‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
---
22
# readability-function-cognitive-complexity temporarily disabled until clang-tidy is fixed
33
# right now emalloc causes it to misbehave
4-
Checks: '*,-bugprone-reserved-identifier,-hicpp-signed-bitwise,-llvmlibc-restrict-system-libc-headers,-altera-unroll-loops,-hicpp-named-parameter,-cert-dcl37-c,-cert-dcl51-cpp,-read,-cppcoreguidelines-init-variables,-cppcoreguidelines-avoid-non-const-global-variables,-altera-id-dependent-backward-branch,-performance-no-int-to-ptr,-altera-struct-pack-align,-google-readability-casting,-modernize-use-trailing-return-type,-llvmlibc-implementation-in-namespace,-llvmlibc-callee-namespace,-cppcoreguidelines-pro-bounds-pointer-arithmetic,-fuchsia-default-arguments-declarations,-fuchsia-overloaded-operator,-cppcoreguidelines-pro-type-union-access,-fuchsia-default-arguments-calls,-cppcoreguidelines-non-private-member-variables-in-classes,-misc-non-private-member-variables-in-classes,-google-readability-todo,-llvm-header-guard,-readability-function-cognitive-complexity,-readability-identifier-length,-modernize-macro-to-enum,-misc-include-cleaner,-bugprone-empty-catch,-cppcoreguidelines-avoid-do-while,-hicpp-no-array-decay'
4+
Checks: '*,-bugprone-reserved-identifier,-hicpp-signed-bitwise,-llvmlibc-restrict-system-libc-headers,-altera-unroll-loops,-hicpp-named-parameter,-cert-dcl37-c,-cert-dcl51-cpp,-read,-cppcoreguidelines-init-variables,-cppcoreguidelines-avoid-non-const-global-variables,-altera-id-dependent-backward-branch,-performance-no-int-to-ptr,-altera-struct-pack-align,-google-readability-casting,-modernize-use-trailing-return-type,-llvmlibc-implementation-in-namespace,-llvmlibc-callee-namespace,-cppcoreguidelines-pro-bounds-pointer-arithmetic,-fuchsia-default-arguments-declarations,-fuchsia-overloaded-operator,-cppcoreguidelines-pro-type-union-access,-fuchsia-default-arguments-calls,-cppcoreguidelines-non-private-member-variables-in-classes,-misc-non-private-member-variables-in-classes,-google-readability-todo,-llvm-header-guard,-readability-function-cognitive-complexity,-readability-identifier-length,-modernize-macro-to-enum,-misc-include-cleaner,-bugprone-empty-catch,-cppcoreguidelines-avoid-do-while,-hicpp-no-array-decay,-llvmlibc-*'
55
WarningsAsErrors: '*'
66
HeaderFilterRegex: ''
77
CheckOptions:

‎appsec/src/helper/main.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ bool ensure_unique(const std::string &lock_path)
8080
// If we fail to obtain the lock, for whichever reason, assume we can't
8181
// run for now.
8282
if (res == -1) {
83+
::close(fd);
8384
SPDLOG_INFO("Failed to get exclusive lock on file {}: errno {}",
8485
lock_path, errno);
8586
return false;

‎appsec/src/helper/network/acceptor.cpp‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,10 @@ using namespace std::chrono_literals;
2121
namespace dds::network::local {
2222

2323
acceptor::acceptor(const std::string_view &sv)
24-
// NOLINTNEXTLINE(android-cloexec-socket)
25-
: sock_(::socket(AF_UNIX, SOCK_STREAM, 0))
2624
{
27-
if (sock_ == -1) {
25+
// NOLINTNEXTLINE(android-cloexec-socket,cppcoreguidelines-prefer-member-initializer)
26+
sock_ = owned_fd{::socket(AF_UNIX, SOCK_STREAM, 0)};
27+
if (sock_.is_empty()) {
2828
throw std::system_error(errno, std::generic_category());
2929
}
3030

@@ -45,7 +45,8 @@ acceptor::acceptor(const std::string_view &sv)
4545

4646
res =
4747
// NOLINTNEXTLINE
48-
::bind(sock_, reinterpret_cast<struct sockaddr *>(&addr), sizeof(addr));
48+
::bind(sock_.get(), reinterpret_cast<struct sockaddr *>(&addr),
49+
sizeof(addr));
4950
if (res == -1) {
5051
SPDLOG_ERROR(
5152
"Failed to bind socket to {}: errno {}", addr.sun_path, errno);
@@ -60,7 +61,7 @@ acceptor::acceptor(const std::string_view &sv)
6061
}
6162

6263
static constexpr int backlog = 50;
63-
if (::listen(sock_, backlog) == -1) {
64+
if (::listen(sock_.get(), backlog) == -1) {
6465
throw std::system_error(errno, std::generic_category());
6566
}
6667
SPDLOG_INFO("Started listening on {}", sv);
@@ -69,7 +70,8 @@ acceptor::acceptor(const std::string_view &sv)
6970
void acceptor::set_accept_timeout(std::chrono::seconds timeout)
7071
{
7172
struct timeval tv = {timeout.count(), 0};
72-
int const res = setsockopt(sock_, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
73+
int const res =
74+
setsockopt(sock_.get(), SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
7375
if (res == -1) {
7476
throw std::system_error(errno, std::generic_category());
7577
}
@@ -80,8 +82,9 @@ std::unique_ptr<base_socket> acceptor::accept()
8082
struct sockaddr_un addr {};
8183
socklen_t len = sizeof(addr);
8284

83-
// NOLINTNEXTLINE
84-
int s = ::accept(sock_, reinterpret_cast<struct sockaddr *>(&addr), &len);
85+
int s =
86+
// NOLINTNEXTLINE
87+
::accept(sock_.get(), reinterpret_cast<struct sockaddr *>(&addr), &len);
8588
if (s == -1) {
8689
if (errno == EINTR || errno == EAGAIN) {
8790
return {};

‎appsec/src/helper/network/acceptor.hpp‎

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
// (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc.
66
#pragma once
77

8+
#include "../utils.hpp"
89
#include "socket.hpp"
910
#include <chrono>
1011
#include <string_view>
@@ -28,35 +29,16 @@ namespace local {
2829

2930
class acceptor : public base_acceptor {
3031
public:
31-
explicit acceptor(int fd) : sock_{fd} {};
32+
explicit acceptor(owned_fd fd) : sock_{std::move(fd)} {};
3233
explicit acceptor(const std::string_view &sv);
3334
acceptor(const acceptor &) = delete;
3435
acceptor &operator=(const acceptor &) = delete;
3536

36-
acceptor(acceptor &&other) noexcept : sock_(other.sock_)
37-
{
38-
other.sock_ = -1;
39-
}
40-
41-
acceptor &operator=(acceptor &&other) noexcept
42-
{
43-
sock_ = other.sock_;
44-
other.sock_ = -1;
45-
return *this;
46-
}
47-
48-
~acceptor() override
49-
{
50-
if (sock_ != -1) {
51-
close(sock_);
52-
}
53-
}
54-
5537
void set_accept_timeout(std::chrono::seconds timeout) override;
5638
[[nodiscard]] std::unique_ptr<base_socket> accept() override;
5739

5840
private:
59-
int sock_{-1};
41+
owned_fd sock_;
6042
};
6143

6244
} // namespace local

‎appsec/src/helper/runner.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ std::unique_ptr<network::base_acceptor> acceptor_from_config(
4444
throw std::invalid_argument{
4545
"fd specified on config is invalid or no socket"};
4646
}
47-
return std::make_unique<network::local::acceptor>(fd);
47+
return std::make_unique<network::local::acceptor>(dds::owned_fd{fd});
4848
}
4949

5050
return std::make_unique<network::local::acceptor>(sock_path);

‎appsec/src/helper/utils.hpp‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,4 +65,31 @@ inline std::string strerror_ts(int errnum)
6565
return buf;
6666
}
6767

68+
class owned_fd {
69+
public:
70+
owned_fd() = default;
71+
explicit owned_fd(int fd) : fd_{fd} {}
72+
owned_fd(const owned_fd &) = delete;
73+
owned_fd &operator=(const owned_fd &) = delete;
74+
owned_fd(owned_fd &&other) noexcept : fd_{other.fd_} { other.fd_ = -1; }
75+
owned_fd &operator=(owned_fd &&other) noexcept
76+
{
77+
fd_ = other.fd_;
78+
other.fd_ = -1;
79+
return *this;
80+
}
81+
~owned_fd()
82+
{
83+
if (fd_ != -1) {
84+
::close(fd_);
85+
}
86+
}
87+
88+
int get() const { return fd_; }
89+
bool is_empty() const { return fd_ == -1; }
90+
91+
private:
92+
int fd_{-1};
93+
};
94+
6895
} // namespace dds

‎appsec/src/helper/worker_pool.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@ void work_handler(queue_consumer &&q, std::optional<runnable> &&opt_r)
2222
// NOLINTNEXTLINE(bugprone-unchecked-optional-access)
2323
opt_r.value()(q);
2424

25+
// Clear the optional to reclaim any "resources", such as file
26+
// descriptors
27+
opt_r.reset();
28+
2529
opt_r = q.pop(consumer_timeout);
2630
}
2731
}

0 commit comments

Comments
 (0)