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.

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

Adding networking.h to provide RAII and iterator support for networking calls #497

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

keith-horton
Copy link
Member

Additions:

  1. I #include’d network headers in the “special order” that gives access to all functions and structures
    a. This is because most networking headers follow the horrible practice of tight inter-dependencies
    i. I.e., networking headers will have #ifdef’s hiding functions & structures requiring you previously included some other header.
    ii. Rinse & repeat this process across a few headers, and you see a common grievance: “what’s the magical header includes to use to get this dang thing to compile!”
    b. Currently I just #include’d them all by default.
    i. I was tempted to only #include them all if the caller defined a special #define before including my header.
    c. I’m very much interesting in thoughts (especially by those who have had to deal with header inclusion issues with other wil headers)
    i. Additionally some #if guards to help developers avoid the confusing mess of Winsock 1.1 and Winsock 2.2. headers (they cannot both be included). [and by default Windows.h includes the old 1.1 headers …. Something that breaks almost every Windows network developer at some point]

  2. 2 structures to load function pointers that are somewhat unpleasant to deal with.
    a. Winsock has many extensibility models – almost all being a huge PITA and failed miserably.
    b. One of the extensibility models is to expose “extension functions” (functions added after the Winsock APIs were agreed upon) by calling a Winsock IOCTL function (WSAIoctl) with special GUID values to receive function pointers for newer functions.
    c. I wrote 2 fairly straight forward structures that will load all of this stuff and just expose a struct with those function pointers.
    d. E.g. (from a test case)
    ::wil::networking::winsock_extension_function_table test_table = ::wil::networking::winsock_extension_function_table::load();
    .
    .
    .
    auto acceptex_return = test_table.f.AcceptEx(
    listeningSocket.get(),
    acceptSocket.get(),
    acceptex_output_buffer,
    0,
    singleAddressOutputBufferSize,
    singleAddressOutputBufferSize,
    &acceptex_bytes_received,
    &acceptex_overlapped);

e. Notice that I just make the structure within the winsock_extension_function_table visible – with the simple name ‘f’.
f. This looks somewhat kludgy – so I’m open to thoughts on this. I followed the other extension API set (RIO – ‘registered I/O’) that returns a well-defined function table: RIO_EXTENSION_FUNCTION_TABLE.
i. vs. creating methods off of these wil extension classes – mirroring the function declarations in these function tables. Which seemed low value.

  1. A class to encapsulate the most painful structure (imho) in Networking – the sockaddr for IPv4 and IPv6 addresses.
    a. A ‘sockaddr’ is a BSD structure – a 1970’s version of “inheritance” implemented in C. It’s required that all “socket address structures” have the exact same fields as the root ‘sockaddr’ in the exact same layout – then just add on new properties as needed.
    i. For example, for IPv4 addresses there’s a ‘sockaddr_in’; for IPv6 addresses there’s a sockaddr_in6.
    ii. E.g. SOCKADDR_IN (ws2def.h) - Win32 apps | Microsoft Learn, sockaddr(3type) - Linux manual page, Internet Address Formats (The GNU C Library)
    b. If you’ve read much networking code, you might find the strangest/hackiest/full-of-random-C-casts code all dealing with these unpleasant data structures.
    i. This comment from my header might help explain some of the goals:
    //
    // encapsulates working with the sockaddr datatype
    //
    // sockaddr is a generic type - similar to a base class, but designed for C with BSD sockets (1983-ish)
    // 'derived' structures are cast back to sockaddr* (so the initial struct members must be aligned)
    //
    // this data type was built to be 'extensible' so new network types could create their own address structures
    // - appending fields to the initial fields of the sockaddr
    //
    // note that the address and port fields of TCPIP sockaddr* types were designed to be encoded in network-byte order
    // - hence the common use of "host-to-network" and "network-to-host" APIs, e.g. htons(), htonl(), ntohs(), ntohl()
    //
    // Socket APIs that accept a socket address will accept 2 fields:
    // - the sockaddr* (the address of the derived sockaddr type, cast down to a sockaddr*)
    // - the length of the 'derived' socket address structure referenced by the sockaddr*
    //
    // Commonly used sockaddr* types that are using with TCPIP networking:
    //
    // sockaddr_storage / SOCKADDR_STORAGE
    // - a sockaddr* derived type that is guaranteed to be large enough to hold any possible socket address
    // sockaddr_in / SOCKADDR_IN
    // - a sockaddr* derived type designed to contain an IPv4 address and port number
    // sockaddr_in6 / SOCKADDR_IN6
    // - a sockaddr* derived type designed to contain an IPv6 address, port, scope id, and flow info
    // SOCKADDR_INET
    // - a union of sockaddr_in and sockaddr_in6 -- i.e., large enough to contain any TCPIP address
    // in_addr / IN_ADDR
    // - the raw address portion of a sockaddr_in
    // in6_addr / IN6_ADDR
    // - the raw address portion of a sockaddr_in6
    //
    // SOCKET_ADDRESS
    // - not a derived sockaddr* type
    // - a structure containing both a sockaddr* and its length fields, returned from some networking functions
    //
    c. The class is hopefully straight forward – it handles many of the unpleasantries moving between types, dealing with embedded types, proving type-safe accessors, and in handling the cases when one must deal with the unpleasant task of knowing when & how to convert between host-byte-order and network-byte-order and vice versa.
    i. Many ways to construct/set_* the IP address in the class – including from a string expression of the address.
    ii. Setting other properties of an address (port, scope id, flow info).
    iii. Writing out an address to either a WCHAR[] or CHAR[] guaranteed to be large enough to hold the string (so no dynamic memory allocation – just a small variable pushed on the stack); as well as a std::wstring if that’s what’s requested.
    iv. Providing many accessors to retrieve various pointers into the underlying sockaddr.

  2. A small class that provides RAII + iterator semantics for walking the returned list of IP addresses after resolving a name:

    //! class addr_info encapsulates the ADDRINFO structure
    //! this structure contains a linked list of addresses returned from resolving a name via GetAddrInfo
    //! iterator semantics are supported to safely access these addresses
    class addr_info

a. And free standing functions to call the underlying API to resolve a name – returning the addr_info containing the response.
b. E.g. a few lines of code makes a tool to dump out local addresses, and prints out addresses when resolving a name:

#include
#include "c:/Users/khorton/source/repos/wil-keith-horton/include/wil/networking.h"

int wmain(int argc, wchar_t** argv)
try
{
if (argc != 2)
{
return 0;
}

const auto wsa_cleanup = wil::networking::WSAStartup();

wprintf(L"Local addresses:\n");
for (const auto& address : wil::networking::resolve_local_addresses())
{
wprintf(L" - %ws\n", address.write_address().c_str());
}

wprintf(L"\nResolving %ws:\n", argv[1]);
for (const auto& address : wil::networking::resolve_name(argv[1]))
{
wprintf(L" - %ws\n", address.write_address().c_str());
}
}
catch (const std::exception& e)
{
wprintf(L"\nFailed - %hs\n", e.what());
}

C:\Users\khorton\source\repos\test\x64\Debug>test.exe microsoft.com
Local addresses:

  • fe80::fa5c:5327:18fe:5e37
  • fe80::7218:2b8a:195b:450c
  • fe80::2089:24a5:515b:c06
  • 192.168.1.27
  • 172.17.112.1

Resolving microsoft.com:

  • 20.70.246.20
  • 20.76.201.171
  • 20.112.250.133
  • 20.231.239.246
  • 20.236.44.162

@keith-horton
Copy link
Member Author

keith-horton commented Feb 3, 2025

@dunhor , do you know who owns the ComTests? This is failing on the build machines in the "RPC timeout test" -- I don't know why this COM mechanism is not working on the build machines - as it's running correctly on my system when I run the test locally. (all tests passed on all build flavors).

The only change was adding some whitespace because I ran clang-format --> and the script ended up formatting those other files.

edit: I added a comment where the test failed after I looked up what that failure was.

@dmachaj
Copy link
Contributor

dmachaj commented Feb 3, 2025

@dunhor , do you know who owns the ComTests? This is failing on the build machines in the "RPC timeout test" -- I don't know why this COM mechanism is not working on the build machines - as it's running correctly on my system when I run the test locally. (all tests passed on all build flavors).

The only change was adding some whitespace because I ran clang-format --> and the script ended up formatting those other files.

edit: I added a comment where the test failed after I looked up what that failure was.

I don't own the ComTests as a whole but the timeout test is one that I added (see #459). It was then tweaked significantly by someone else in #477. This concept has proven challenging to test reliably :(. I'm not sure if the timeouts simply need to be extended by a bit or if there is a deeper race somewhere.

…n in the pipeline. Addressed one test that caused address sanitizer to fail - now all the below tests consistently pass :)

c:\Users\khorton\source\repos\wil-keith-horton\build\clang64debug\tests\cpplatest\witest.cpplatest.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\clang64debug\tests\noexcept\witest.noexcept.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\clang64debug\tests\normal\witest.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\clang64debug\tests\win7\witest.win7.exe

c:\Users\khorton\source\repos\wil-keith-horton\build\clang64relwithdebinfo\tests\cpplatest\witest.cpplatest.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\clang64relwithdebinfo\tests\noexcept\witest.noexcept.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\clang64relwithdebinfo\tests\normal\witest.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\clang64relwithdebinfo\tests\win7\witest.win7.exe

c:\Users\khorton\source\repos\wil-keith-horton\build\msvc64debug\tests\cpplatest\witest.cpplatest.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\msvc64debug\tests\noexcept\witest.noexcept.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\msvc64debug\tests\normal\witest.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\msvc64debug\tests\win7\witest.win7.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\msvc64debug\tests\sanitize-address\witest.asan.exe

c:\Users\khorton\source\repos\wil-keith-horton\build\msvc64relwithdebinfo\tests\cpplatest\witest.cpplatest.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\msvc64relwithdebinfo\tests\noexcept\witest.noexcept.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\msvc64relwithdebinfo\tests\normal\witest.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\msvc64relwithdebinfo\tests\win7\witest.win7.exe
c:\Users\khorton\source\repos\wil-keith-horton\build\msvc64relwithdebinfo\tests\sanitize-address\witest.asan.exe
@keith-horton
Copy link
Member Author

@dunhor , I've run scripts\run-clang-format.cmd and checked that in -- but the build still fails saying I need to run clang-format :(

I'm running scripts\run-clang-format.cmd -- but no files are edited.
Should I be running something else?

(also I think I fixed the COM tests ... I left printf calls there for future folks that need to debug)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants