Skip to content
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

Jump to bottom

Remove Ip::Address implicit sockaddr_in6 conversion #1823

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+71 −82
Open

Remove Ip::Address implicit sockaddr_in6 conversion #1823

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ddca48a
Remove Ip::Address implicit sockaddr_in6 conversion
yadij May 28, 2024
fadedfb
Update src/ip/Address.h
yadij May 29, 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
28 changes: 9 additions & 19 deletions src/ip/Address.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,12 @@ Ip::Address::operator =(struct sockaddr_in const &s)
return *this;
};

Ip::Address::Address(struct sockaddr_storage const &s)
{
setEmpty();
operator=(s);
};

Ip::Address &
Ip::Address::operator =(const struct sockaddr_storage &s)
{
Expand All @@ -459,19 +465,6 @@ Ip::Address::operator =(const struct sockaddr_storage &s)
return *this;
};

Ip::Address::Address(struct sockaddr_in6 const &s)
{
setEmpty();
operator=(s);
};

Ip::Address &
Ip::Address::operator =(struct sockaddr_in6 const &s)
{
memmove(&mSocketAddr_, &s, sizeof(struct sockaddr_in6));
return *this;
};

Ip::Address::Address(struct in_addr const &s)
{
setEmpty();
Expand Down Expand Up @@ -557,8 +550,6 @@ Ip::Address::operator =(const struct addrinfo &s)

struct sockaddr_in* ipv4 = nullptr;

struct sockaddr_in6* ipv6 = nullptr;

//struct addrinfo {
// int ai_flags; /* input flags */
// int ai_family; /* protocol family for socket */
Expand All @@ -580,10 +571,9 @@ Ip::Address::operator =(const struct addrinfo &s)
break;

case AF_INET6:
ipv6 = (sockaddr_in6*)(s.ai_addr);
/* this */
assert(ipv6);
operator=(*ipv6);
assert(s.ai_addr);
memmove(&mSocketAddr_, s.ai_addr, sizeof(struct sockaddr_in6));
break;

case AF_UNSPEC:
Expand All @@ -592,7 +582,7 @@ Ip::Address::operator =(const struct addrinfo &s)
// such as those where data only comes from getsockopt()
if (s.ai_addr != nullptr) {
if (s.ai_addrlen == sizeof(struct sockaddr_in6)) {
operator=(*((struct sockaddr_in6*)s.ai_addr));
memmove(&mSocketAddr_, s.ai_addr, s.ai_addrlen);
return true;
} else if (s.ai_addrlen == sizeof(struct sockaddr_in)) {
operator=(*((struct sockaddr_in*)s.ai_addr));
Expand Down
3 changes: 1 addition & 2 deletions src/ip/Address.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ class Address
/** @name Constructors */
/*@{*/
Address() { setEmpty(); }
Address(const struct sockaddr_storage &);
yadij marked this conversation as resolved.
Show resolved Hide resolved
Address(const struct in_addr &);
Address(const struct sockaddr_in &);
Address(const struct in6_addr &);
Address(const struct sockaddr_in6 &);
Copy link
Contributor

Choose a reason for hiding this comment

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

I support adding a sockaddr_storage constructor, but why remove this constructor? What is wrong with it (other than the fact that it allows implicit conversions -- a problem that should be fixed by adding "explicit" rather than by removing)?

PR description says "Migration to sockaddr_storage API", but that phrase does not answer my question (and raises more red flags). If that phrase was meant to explain why sockaddr_in6 conversion constructor is removed, please rephrase. Otherwise, please remove that PR description phrase completely, so that we do not have to argue about (and adjust it to clarify) its meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This constructor (and matching assignment operator) are no longer used after this PR updates the callers to sockaddr_storage.

Copy link
Contributor

Choose a reason for hiding this comment

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

This constructor (and matching assignment operator) are no longer used

... but their code is still there; it is just not wrapped in a Address methods anymore and is now duplicated and hidden. I was going to flag that problem in later review iterations (if still necessary)...

For now, I was hoping that you will agree to restore those methods, preserve Ip::Address API symmetry with respect to IPv4/IPv6, and focus on prohibiting implicit conversions and/or on adding explicit sockaddr_storage conversion support. The last two are good goals that can be accomplished without removing those IPv6-specific methods.

Address(const struct hostent &);
Address(const struct addrinfo &);
Address(const char*);
Expand All @@ -59,7 +59,6 @@ class Address
Address& operator =(struct sockaddr_storage const &s);
Address& operator =(struct in_addr const &s);
Address& operator =(struct in6_addr const &s);
Address& operator =(struct sockaddr_in6 const &s);
bool operator =(const struct hostent &s);
bool operator =(const struct addrinfo &s);
bool operator =(const char *s);
Expand Down
3 changes: 1 addition & 2 deletions src/tests/stub_libip.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,17 @@
#include "tests/STUB.h"

#include "ip/Address.h"
Ip::Address::Address(const struct sockaddr_storage &) STUB
Ip::Address::Address(const struct in_addr &) STUB
Ip::Address::Address(const struct sockaddr_in &) STUB
Ip::Address::Address(const struct in6_addr &) STUB
Ip::Address::Address(const struct sockaddr_in6 &) STUB
Ip::Address::Address(const struct hostent &) STUB
Ip::Address::Address(const struct addrinfo &) STUB
Ip::Address::Address(const char*) STUB
Ip::Address& Ip::Address::operator =(struct sockaddr_in const &) STUB_RETVAL(*this)
Ip::Address& Ip::Address::operator =(struct sockaddr_storage const &) STUB_RETVAL(*this)
Ip::Address& Ip::Address::operator =(struct in_addr const &) STUB_RETVAL(*this)
Ip::Address& Ip::Address::operator =(struct in6_addr const &) STUB_RETVAL(*this)
Ip::Address& Ip::Address::operator =(struct sockaddr_in6 const &) STUB_RETVAL(*this)
bool Ip::Address::operator =(const struct hostent &) STUB_RETVAL(false)
bool Ip::Address::operator =(const struct addrinfo &) STUB_RETVAL(false)
bool Ip::Address::operator =(const char *) STUB_RETVAL(false)
Expand Down
119 changes: 60 additions & 59 deletions src/tests/testIpAddress.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ class TestIpAddress : public CPPUNIT_NS::TestFixture
CPPUNIT_TEST(testInAddrConstructor);
CPPUNIT_TEST(testInAddr6Constructor);
CPPUNIT_TEST(testSockAddrConstructor);
CPPUNIT_TEST(testSockAddr6Constructor);
CPPUNIT_TEST(testHostentConstructor);
CPPUNIT_TEST(testStringConstructor);
CPPUNIT_TEST(testCopyConstructor);
Expand All @@ -59,7 +58,6 @@ class TestIpAddress : public CPPUNIT_NS::TestFixture
void testInAddrConstructor();
void testInAddr6Constructor();
void testSockAddrConstructor();
void testSockAddr6Constructor();
void testHostentConstructor();
void testStringConstructor();
void testCopyConstructor();
Expand Down Expand Up @@ -144,62 +142,64 @@ TestIpAddress::testInAddr6Constructor()
void
TestIpAddress::testSockAddrConstructor()
{
struct sockaddr_in insock;
struct sockaddr_in outsock;
// sockaddr_storage containing sockaddr_in
{
struct sockaddr_storage ss = {};
auto *insock = reinterpret_cast<struct sockaddr_in *>(&ss);
Copy link
Contributor

@rousskov rousskov May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I hope this particular code will disappear due to another change request, but the idea expressed here applies to (another area in) adjusted code as well.

If possible, please avoid never-nil pointers to local objects and reduce the difference with the current code:

Suggested change
auto *insock = reinterpret_cast<struct sockaddr_in *>(&ss);
auto &insock = reinterpret_cast<struct sockaddr_in &>(ss);

Same for the other similar changes in this PR, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I will do once that other discussion is resolved.


memset(&insock, 0, sizeof(struct sockaddr_in));
memset(&outsock, 0, sizeof(struct sockaddr_in));
struct sockaddr_in outsock;
memset(&outsock, 0, sizeof(struct sockaddr_in));

insock.sin_family = AF_INET;
insock.sin_port = htons(80);
insock.sin_addr.s_addr = htonl(0xC0A8640C);
insock->sin_family = AF_INET;
insock->sin_port = htons(80);
insock->sin_addr.s_addr = htonl(0xC0A8640C);
#if HAVE_SIN_LEN_IN_SAI
insock.sin_len = sizeof(struct sockaddr_in);
insock->sin_len = sizeof(struct sockaddr_in);
#endif

Ip::Address anIPA(insock);
Ip::Address anIPA(ss);

/* test stored values */
CPPUNIT_ASSERT(!anIPA.isAnyAddr());
CPPUNIT_ASSERT(!anIPA.isNoAddr());
CPPUNIT_ASSERT(anIPA.isIPv4());
CPPUNIT_ASSERT(!anIPA.isIPv6());
CPPUNIT_ASSERT(anIPA.isSockAddr());
CPPUNIT_ASSERT_EQUAL(static_cast<unsigned short>(80), anIPA.port());
anIPA.getSockAddr(outsock);
CPPUNIT_ASSERT(memcmp(insock, &outsock, sizeof(struct sockaddr_in)) == 0);
}

/* test stored values */
CPPUNIT_ASSERT( !anIPA.isAnyAddr() );
CPPUNIT_ASSERT( !anIPA.isNoAddr() );
CPPUNIT_ASSERT( anIPA.isIPv4() );
CPPUNIT_ASSERT( !anIPA.isIPv6() );
CPPUNIT_ASSERT( anIPA.isSockAddr() );
CPPUNIT_ASSERT_EQUAL( (unsigned short) 80, anIPA.port() );
anIPA.getSockAddr(outsock);
CPPUNIT_ASSERT( memcmp( &insock, &outsock, sizeof(struct sockaddr_in)) == 0 );
}
// sockaddr_storage containing sockaddr_in6
{
struct sockaddr_storage ss = {};
auto *insock = reinterpret_cast<struct sockaddr_in6 *>(&ss);

void
TestIpAddress::testSockAddr6Constructor()
{
struct sockaddr_in6 insock;
struct sockaddr_in6 outsock;

memset(&insock, 0, sizeof(struct sockaddr_in6));
memset(&outsock, 0, sizeof(struct sockaddr_in6));

insock.sin6_family = AF_INET6;
insock.sin6_port = htons(80);
insock.sin6_addr.s6_addr32[0] = htonl(0xFFFFFFFF);
insock.sin6_addr.s6_addr32[1] = htonl(0x00000000);
insock.sin6_addr.s6_addr32[2] = htonl(0x0000FFFF);
insock.sin6_addr.s6_addr32[3] = htonl(0xC0A8640C);
struct sockaddr_in6 outsock;
memset(&outsock, 0, sizeof(struct sockaddr_in6));

insock->sin6_family = AF_INET6;
insock->sin6_port = htons(80);
insock->sin6_addr.s6_addr32[0] = htonl(0xFFFFFFFF);
insock->sin6_addr.s6_addr32[1] = htonl(0x00000000);
insock->sin6_addr.s6_addr32[2] = htonl(0x0000FFFF);
insock->sin6_addr.s6_addr32[3] = htonl(0xC0A8640C);
#if HAVE_SIN6_LEN_IN_SAI
insock.sin6_len = sizeof(struct sockaddr_in6);
insock->sin6_len = sizeof(struct sockaddr_in6);
#endif

Ip::Address anIPA((const struct sockaddr_in6)insock);

/* test stored values */
CPPUNIT_ASSERT( !anIPA.isAnyAddr() );
CPPUNIT_ASSERT( !anIPA.isNoAddr() );
CPPUNIT_ASSERT( !anIPA.isIPv4() );
CPPUNIT_ASSERT( anIPA.isIPv6() );
CPPUNIT_ASSERT( anIPA.isSockAddr() );
CPPUNIT_ASSERT_EQUAL( (unsigned short) 80, anIPA.port() );
anIPA.getSockAddr(outsock);
CPPUNIT_ASSERT( memcmp( &insock, &outsock, sizeof(struct sockaddr_in6)) == 0 );
Ip::Address anIP(ss);

/* test stored values */
CPPUNIT_ASSERT(!anIP.isAnyAddr());
CPPUNIT_ASSERT(!anIP.isNoAddr());
CPPUNIT_ASSERT(!anIP.isIPv4());
CPPUNIT_ASSERT(anIP.isIPv6());
CPPUNIT_ASSERT(anIP.isSockAddr());
CPPUNIT_ASSERT_EQUAL(static_cast<unsigned short>(80), anIP.port());
anIP.getSockAddr(outsock);
CPPUNIT_ASSERT(memcmp(insock, &outsock, sizeof(struct sockaddr_in6)) == 0);
}
}

void
Expand Down Expand Up @@ -532,20 +532,21 @@ TestIpAddress::testtoUrl_fromSockAddr()
anIPA.toUrl(buf,MAX_IPSTRLEN);
CPPUNIT_ASSERT( memcmp("192.168.100.12:80", buf, 17) == 0 );

/* test output when constructed from in6_addr with IPv6 */
struct sockaddr_in6 ip6val;

ip6val.sin6_addr.s6_addr32[0] = htonl(0xC0A8640C);
ip6val.sin6_addr.s6_addr32[1] = htonl(0xFFFFFFFF);
ip6val.sin6_addr.s6_addr32[2] = htonl(0xFFFFFFFF);
ip6val.sin6_addr.s6_addr32[3] = htonl(0xFFFFFFFF);
ip6val.sin6_port = htons(80);
ip6val.sin6_family = AF_INET6;
/* test output when constructed from sockaddr_storage with IPv6 */
struct sockaddr_storage ss;
auto *ip6val = reinterpret_cast<struct sockaddr_in6*>(&ss);
Comment on lines +536 to +537
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do the opposite -- continue to use sockaddr_in6 ipv6val definition and then just cast ipv6 to a sockaddr_storage reference at the time of conversion to Ip::Address? Doing so would simplify and clarify this code. Right now, code further below appears to configure one variable (ipv6val) but use another (ss). If we do not have to use sockaddr_storage as ip6val storage while configuring ipv6val, then let's remove that complication.

Suggested change
struct sockaddr_storage ss;
auto *ip6val = reinterpret_cast<struct sockaddr_in6*>(&ss);
struct sockaddr_in6 ip6val;

... and adjust Ip::Address construction accordingly.

Same for other similar test cases, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not without adding more technical debt for the future.
sockaddr_storage is defined as being large enough to store the largest sockaddr_* type. That includes experimental IPv7+ address types and some edge-cases like interface-scoped IPv6. If we went with sockaddr_in6 we would lock Squid to only supporting "pure" IPv6.

I would like to get the API as minimal as possible without locking out capabilities like that. But not to go as far as a C library with sockaddr * raw-pointers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not without adding more technical debt for the future.

I do not see any technical debt. I suspect that my suggestion was misinterpreted.

I now also realize that the second part of my suggestion was wrong -- one must not cast a sockaddr_in6 variable to sockaddr_storage. Only the opposite cast is safe!

sockaddr_storage is defined as being large enough to store the largest sockaddr_* type. That includes experimental IPv7+ address types and some edge-cases like interface-scoped IPv6. If we went with sockaddr_in6 we would lock Squid to only supporting "pure" IPv6.

I see no relationship between the facts mentioned above and my suggestion (that we should (continue to) use sockaddr_in6 type to create a sockaddr_in6 address for the test case). We obviously want to test with Ip::Address with IPv6 addresses. Let's (continue to) use the corresponding sockaddr_in6 type for that test. There is no reason to outlaw that type; we ought to support it at least until the last sockaddr_in6 expression is gone from Squid primary code.

  • In this PR, this code should continue to (naturally) configure its IPv6 input using sockaddr_in6.

  • In a future PR, when we add support for experimental IPv7 addresses, the test code will (naturally) configure its IPv7 input using sockaddr_in7.

We can also add sockaddr_storage tests, of course. They can (and probably should) reuse the bulk of existing sockaddr_in6 and sockaddr_in test code, memcopying configured sockaddr_in... addresses to sockaddr_storage variables.

I would like to get the API as minimal as possible without locking out capabilities like that.

No API changes or "locking out capabilities" suggested in this change request.


ip6val->sin6_addr.s6_addr32[0] = htonl(0xC0A8640C);
ip6val->sin6_addr.s6_addr32[1] = htonl(0xFFFFFFFF);
ip6val->sin6_addr.s6_addr32[2] = htonl(0xFFFFFFFF);
ip6val->sin6_addr.s6_addr32[3] = htonl(0xFFFFFFFF);
ip6val->sin6_port = htons(80);
ip6val->sin6_family = AF_INET6;
#if HAVE_SIN6_LEN_IN_SAI
ip6val.sin6_len = sizeof(struct sockaddr_in6);
ip6val->sin6_len = sizeof(struct sockaddr_in6);
#endif

Ip::Address bnIPA(ip6val);
Ip::Address bnIPA(ss);
Copy link
Contributor

@rousskov rousskov May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If possible:

Suggested change
Ip::Address bnIPA(ss);
const Ip::Address bnIPA(reinterpret_cast<struct sockaddr_storage &>(ip6val));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am reluctant to do this one. While it matches up with the earlier IPv4 constructor test case, the intention of this case is to verify that the sockaddr_storage content is correctly interpreted by Ip::Address when being set via an ipv6val pointer/reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will rephrase to avoid contaminating this trivial change request with complex changes from another one:

Suggested change
Ip::Address bnIPA(ss);
const Ip::Address bnIPA(ss);


bnIPA.toUrl(buf,MAX_IPSTRLEN);
CPPUNIT_ASSERT( memcmp("[c0a8:640c:ffff:ffff:ffff:ffff:ffff:ffff]:80", buf, 44) == 0 );
Expand Down
Loading