refactor!: switch to sockaddr_storage, IPv6 and resolver improvements #1140
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #1140 +/- ##
==========================================
- Coverage 78.60% 78.22% -0.39%
==========================================
Files 53 53
Lines 6872 6852 -20
==========================================
- Hits 5402 5360 -42
- Misses 1470 1492 +22
☔ View full report in Codecov by Sentry. |
Tachi107
force-pushed
the
sockaddr-storage
branch
from
ee07d9b
to
8c9c770
Compare
|
Yeah there are a lot of |
Tachi107
force-pushed
the
sockaddr-storage
branch
from
8c9c770
to
065565e
Compare
|
@kiplingw please keep this PR in draft status until I'm confident to let you merge it. I'm not done yet :) |
Tachi107
force-pushed
the
sockaddr-storage
branch
2 times, most recently
from
dfd0848
to
d519ba9
Compare
|
Hey @dennisjenkins75 and @kiplingw, could you please have a look at at the changes? You can find all the relevant descriptions in the various commits. I'm not completely done yet, I'd like to reorder the commit history, for instance. |
|
Sure @Tachi107. Will review when back at my desk. |
|
:( I have only ever written unix sockets code the old way. I was unaware of the newer APIs. I have no problems with the code cleanup so long as it passes all unit tests. |
|
From what I understood of it, it looks fine to me. Good work @Tachi107. Let us know when you are ready and I will re-review before merging. Main thing is, as @dennisjenkins75 said, don't break CI. |
Tachi107
force-pushed
the
sockaddr-storage
branch
from
d519ba9
to
3947e5f
Compare
Tachi107
changed the title
Tachi107
changed the title
|
The commit history is now neatly separated in three independent commits with a somewhat exhaustive explanation about the relevant changes, and I've also added a couple more small tests. Lastly, I bumped the version to 0.2.0, since this patch introduces new features. The commitlint and abidiff failures are expected. @kiplingw I'll patiently wait for your re-review and merge :) |
Using a struct sockaddr_storage allows us to get rid of the spooky union of sockaddr_in and sockaddr_in6 contained in the IP class, and to do IPv4 and IPv6 as recommended by RFC 3493. The code is not that cleaner, but it should be more robust and less "dangerous". As discussed recently on IRC, Kip and I were quite puzzled by the various constructors of class IP, as it wasn't immediatly clear why they were memcpying INET_ADDRSTRLEN bytes into a smaller s_addr array. Needless to say, this is a breaking change. It is impossible to change this without breaking the ABI, and while at it I touched the API as well to clean it up a bit.
Before this commit, Pistache didn't properly support "true" IPv6 addresses, but only IPv6 "host-uri", i.e. a common IPv6 representation used in URIs and protocols like HTTP. With these changes, the AddressParser now understands bare IPv6 addresses in addition to "host-uri" ones, e.g. you can now pass "::1" in addition to "[::1]" to Address' constructor when you only want to specify an IPv6 address without port. Consequently, all the functions which return the textual representation of IPv6 addresses, below the application level (HTTP), now return bare IPv6 addresses, without brackets. Functions which return textual IPv6 + port pairs, return the recommended textual IPv6 form, i.e. [address]:port, which is the same as the URI format.
In this commit I've massively refactored the init() function of the
Address class. It used to carry out most of the address parsing on its
own with a lot of custom code, making a lot of IPv4 vs IPv6 differences
and special casing. It now makes use of the library functions
standardized by RFC 3493, and most of the heavy lifting is done by
getaddrinfo().
This refactor has the following niceties:
1. Less complex code, less chances of bugs.
2. Hooking into getaddrinfo() makes Address capable of accepting
symbolic textual service names in addition to numeric ports, so that
users can now create an Address object like Address("*:http")
3. The loopback and wildcard address are now resolved by the system
resolver as well, hardcoding less and yielding more desireable
results when, for example, a system is configured to operate in IPv4
or IPv6 only mode. Since addresses like "localhost" are now resolved
by the system, it is possible that "::1" is now returned as well as
"127.0.0.1", while Pistache would previously only return the latter.
This is a change in behaviour.
The new functionalities described in points 2 and 3 are covered by new
minimal unit tests.
Tachi107
force-pushed
the
sockaddr-storage
branch
from
912125d
to
0eb0cda
Compare
|
@Tachi107, BSD sockets aren't my strong point because it's been years since I had to directly use them. But from what I can understand it looks fine to me. One question I have for you though is did you try running through valgrind to see if there's any buffer overflows or other memory corruption issues before and after the changes? |
|
Yes, I've run both under valgrind and no memory errors were raised. Note however that CI runs all tests under the memory sanitizer, which similar to valgrind in many ways.
Il 3 luglio 2023 19:46:27 CEST, Kip ***@***.***> ha scritto:
…
@Tachi107, BSD sockets aren't my strong point because it's been years since I had to directly use them. But from what I can understand it looks fine to me. One question I have for you though is did you try running through valgrind to see if there's any buffer overflows or other memory corruption issues before and after the changes?
|
|
On Mon, 2023-07-03 at 13:30 -0700, Andrea Pappacoda wrote:
Yes, I've run both under valgrind and no memory errors were raised.
Note however that CI runs all tests under the memory sanitizer, which
similar to valgrind in many ways.
Perfect. Don't forget to bump d/changelog too.
…--
Kip Warner
OpenPGP signed/encrypted mail preferred
https://www.thevertigo.com
|
Using a
struct sockaddr_storageallows us to get rid of the spooky union ofsockaddr_inandsockaddr_in6, and to do IPv4 and IPv6 as recommended by RFC 3493.The code is not that cleaner, but it should be more robust and less "dangerous". As discussed recently on IRC, Kip and I were quite puzzled by the various constructors of class IP, as it wasn't immediatly clear why they were
memcpyingINET_ADDRSTRLENbytes into a smallers_addrarray.Needless to say, this is a breaking change. I also removed a few now useless functions, but they could be kept if desired.
Note: this is still experimental and incomplete.