Skip to content

Fix type mismatch in new/delete of addrinfo::ai_addr #1671

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jtstrs wants to merge 15 commits into from
Closed

Fix type mismatch in new/delete of addrinfo::ai_addr #1671

Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
fd2e895
Fix memory leak at FreeAddr
jtstrs Feb 12, 2024
6ffe158
Update Address.cc
yadij Feb 13, 2024
ea5d9ee
Encapsulate ai_addr construction/destruction
jtstrs Feb 15, 2024
44cf803
Merge branch 'ip/address_bugfix_FixMemoryLeakAfterReleasingAddress' o…
jtstrs Feb 15, 2024
bb7f0c1
Fix incorrect merge result
jtstrs Feb 15, 2024
4f9cb68
fixup: Do not expose Ip::Address users to ai_addr matters
rousskov Feb 16, 2024
51f4bca
fixup: Use C++ to produce safer, cleaner code
rousskov Feb 16, 2024
5bbc908
fixup: Explain why we should continue to memset() here
rousskov Feb 16, 2024
83267df
fixup: Fixed argument type of new functions
rousskov Feb 16, 2024
d408e8f
fixup: Do not throw when cleaning up
rousskov Feb 16, 2024
b4499ac
fixup: Undo out-of-scope formatting improvements
rousskov Feb 16, 2024
49345e0
fixup: Simplified -- we know what we have allocated
rousskov Feb 16, 2024
f6ff166
fiuxp: More paranoid assertions
rousskov Feb 16, 2024
519cb62
fixup: Documented branch-added Ip::FreeAddrMember() function
rousskov Feb 16, 2024
2cacd16
Fix codestyle requests
jtstrs Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 65 additions & 31 deletions src/ip/Address.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,14 @@
} printf("\n"); assert(b); \
}

namespace Ip {

template <class SockAddrType>
static SockAddrType &AllocateAddrMember(struct addrinfo &);
static void FreeAddrMember(struct addrinfo &);

} // namespace Ip

int
Ip::Address::cidr() const
{
Expand Down Expand Up @@ -634,16 +642,9 @@ Ip::Address::getAddrInfo(struct addrinfo *&dst, int force) const
dst->ai_protocol = IPPROTO_UDP;

if (force == AF_INET6 || (force == AF_UNSPEC && isIPv6()) ) {
dst->ai_addr = (struct sockaddr*)new sockaddr_in6;

memset(dst->ai_addr,0,sizeof(struct sockaddr_in6));

getSockAddr(*((struct sockaddr_in6*)dst->ai_addr));

dst->ai_addrlen = sizeof(struct sockaddr_in6);

dst->ai_family = ((struct sockaddr_in6*)dst->ai_addr)->sin6_family;

auto &ai_addr = AllocateAddrMember<struct sockaddr_in6>(*dst);
getSockAddr(ai_addr);
dst->ai_family = ai_addr.sin6_family;
#if 0
/**
* Enable only if you must and please report to squid-dev if you find a need for this.
Expand All @@ -659,16 +660,9 @@ Ip::Address::getAddrInfo(struct addrinfo *&dst, int force) const
#endif

} else if ( force == AF_INET || (force == AF_UNSPEC && isIPv4()) ) {

dst->ai_addr = (struct sockaddr*)new sockaddr_in;

memset(dst->ai_addr,0,sizeof(struct sockaddr_in));

getSockAddr(*((struct sockaddr_in*)dst->ai_addr));

dst->ai_addrlen = sizeof(struct sockaddr_in);

dst->ai_family = ((struct sockaddr_in*)dst->ai_addr)->sin_family;
auto &ai_addr = AllocateAddrMember<struct sockaddr_in>(*dst);
getSockAddr(ai_addr);
dst->ai_family = ai_addr.sin_family;
} else {
IASSERT("false",false);
}
Expand All @@ -683,32 +677,72 @@ Ip::Address::InitAddr(struct addrinfo *&ai)
}

// remove any existing data.
if (ai->ai_addr) delete ai->ai_addr;

ai->ai_addr = (struct sockaddr*)new sockaddr_in6;
memset(ai->ai_addr, 0, sizeof(struct sockaddr_in6));

ai->ai_addrlen = sizeof(struct sockaddr_in6);
FreeAddrMember(*ai);
Copy link
Contributor

@rousskov rousskov Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also move this call into a new else clause for the above addrinfo allocation if statement.


AllocateAddrMember<struct sockaddr_in6>(*ai);
}

void
Ip::Address::FreeAddr(struct addrinfo *&ai)
{
if (ai == nullptr) return;

if (ai->ai_addr) delete ai->ai_addr;

ai->ai_addr = nullptr;

ai->ai_addrlen = 0;
FreeAddrMember(*ai);

// NP: name fields are NOT allocated at present.
delete ai;

ai = nullptr;
}

/// Allocates and fills with zeros a SockAddrType object.
/// Initializes addrinfo::ai_addr and ai_addrlen with the allocated object.
/// \returns the allocated object
template <class SockAddrType>
SockAddrType &
Ip::AllocateAddrMember(struct addrinfo &ai)
{
static_assert(
std::is_same<SockAddrType, struct sockaddr_in>::value ||
std::is_same<SockAddrType, struct sockaddr_in6>::value,
"FreeAddrMember() supports this addrinfo::ai_addr type");

const auto ai_addr = new SockAddrType;
// We do not use `new SockAddrType{}` above instead of memset() below
// because, since C++14, doing so may not initialize SockAddrType padding.
static_assert(std::is_trivial<SockAddrType>::value, "can memset() this addrinfo::ai_addr type");
memset(ai_addr, 0, sizeof(*ai_addr));

ai.ai_addr = reinterpret_cast<struct sockaddr*>(ai_addr);
ai.ai_addrlen = sizeof(*ai_addr);

return *ai_addr;
}

/// Deallocates addrinfo::ai_addr member of the given structure and adjusts that
/// structure accordingly. Safe for structures with a nil ai_addr member.
/// \sa Ip::AllocateAddrMember()
void
Ip::FreeAddrMember(struct addrinfo &ai)
{
if (!ai.ai_addr)
return;

if (ai.ai_addrlen == sizeof(struct sockaddr_in))
delete reinterpret_cast<struct sockaddr_in*>(ai.ai_addr);
else if (ai.ai_addrlen == sizeof(struct sockaddr_in6))
delete reinterpret_cast<struct sockaddr_in6*>(ai.ai_addr);
else {
debugs(14, DBG_CRITICAL, "ERROR: Squid Bug: Unexpected addrinfo::ai_addr size: " << ai.ai_addrlen <<
Debug::Extra << "sockaddr_in size: " << sizeof(struct sockaddr_in) <<
Debug::Extra << "sockaddr_in6 size: " << sizeof(struct sockaddr_in6));
// leak memory
Copy link
Contributor

@rousskov rousskov Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can assert() instead. Throwing in cleanup code is not a good idea.

Copy link
Contributor

@yadij yadij Feb 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert() would also be a bad idea for now. There are struct sockaddr_un stored for UDS sockets Comm::Connection. They have variable-length allocations.

Copy link
Contributor

@rousskov rousskov Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are struct sockaddr_un stored for UDS sockets Comm::Connection. They have variable-length allocations.

N.B. comm_open_uds() does not use this function (i.e. does not call Ip::Address::InitAddr() and FreeAddr()).

If you know of any Squid code that (directly or indirectly) allocates addrinfo::ai_addr using something other than the two PR-handled types and then calls (directly or indirectly) Ip::Address::FreeAddr(), please point me to it. I could not find it. I hope such code does not exist, but if it does exist, then this PR will need to be changed to reflect its existence. (This PR code will need to be changed for other reasons as well, of course, but I am documenting one of the assumptions that the current PR design relies on and that is not directly related to those other reasons).

There is code that allocates addrinfo::ai_addr using getaddrinfo(), but that code calls freeaddrinfo(), as it should. That code does not directly affect this part of the discussion.

}

ai.ai_addr = nullptr;
ai.ai_addrlen = 0;
}

int
Ip::Address::matchIPAddr(const Ip::Address &rhs) const
{
Expand Down