net: Use GetAdaptersAddresses to get local addresses on Windows #31014

1. 
   laanwj commented at 6:57 pm on October 1, 2024: member

   Instead of a gethostname hack, which is not guaranteed to return all addresses, use the official way of calling GetAdaptersAddresses to get local network addresses on Windows.

   Do the same checks as the UNIX path: interface is up, interface is not loopback.

   Suggested by Ava Chow.

   Addiional changes:

   • Add optional length checking to CService::SetSockAddr calls: In almost all cases (the only exception is getifaddrs), we know the size of the data passed into SetSockAddr, so we can check this to be what is expected, before reading the memory.
   • Cleanup: move out FromSockAddr in netif.cpp from MacOS and use it everywhere appropriate. This avoids code duplication.
2. laanwj added the label Windows on Oct 1, 2024
3. laanwj added the label P2P on Oct 1, 2024
4. 
   DrahtBot commented at 6:57 pm on October 1, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31014.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	sipa
   Concept ACK	1440000bytes, jonatack

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31676 (fuzz: add targets for PCP and NAT-PMP port mapping requests by darosior)
   • #31022 (test: Add mockable steady clock, tests for PCP and NATPMP implementations by laanwj)
   • #30988 (Split CConnman by vasild)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

5. laanwj force-pushed on Oct 1, 2024
6. laanwj force-pushed on Oct 1, 2024
7. laanwj force-pushed on Oct 1, 2024
8. laanwj force-pushed on Oct 1, 2024
9. laanwj force-pushed on Oct 1, 2024
10. laanwj force-pushed on Oct 2, 2024
11. DrahtBot added the label Needs rebase on Oct 2, 2024
12. laanwj force-pushed on Oct 3, 2024
13. DrahtBot removed the label Needs rebase on Oct 3, 2024
14. 
    1440000bytes commented at 8:29 pm on October 3, 2024: none

    Instead of a gethostname hack, use the official way of calling GetAdaptersAddresses to get local network addresses on Windows.

    Concept ACK

15. laanwj force-pushed on Oct 25, 2024
16. in src/common/netif.cpp:287 in 135e8b2971 outdated

    268@@ -276,9 +269,46 @@ std::vector<CNetAddr> GetLocalAddresses()
    269 {
    270     std::vector<CNetAddr> addresses;
    271 #ifdef WIN32
    272-    char pszHostName[256] = "";
    273-    if (gethostname(pszHostName, sizeof(pszHostName)) != SOCKET_ERROR) {
    274-        addresses = LookupHost(pszHostName, 0, true);
    275+    DWORD status = 0;
    276+    constexpr size_t MAX_ADAPTER_ADDR_SIZE = 4 * 1000 * 1000; // Absolute maximum size of adapter addresses structure we're willing to handle, as a precaution.
    

    ---

    ---

    

    laanwj commented at 11:12 am on October 25, 2024:

    Added an upper limit here to avoid hanging forever or consuming absurd amounts of resources in case of a windows bug. i would imagine 4MB of adapter addresses ought to be enough for anyone.
17. 
    laanwj commented at 1:04 pm on January 13, 2025: member

    Can anyone review here please?

    Mind that the first commit “Add optional length checking to CService::SetSockAddr” is also part of #31022.

    Edit: needs rebasing for #31022 (review) assuming that goes in first Edit.2: done, took over the same commit, network address length determination logic was moved to FromSockAddr for the unknown-length case

18. net: Add optional length checking to CService::SetSockAddr

    In almost all cases (the only exception is `getifaddrs`), we know the
    size of the data passed into SetSockAddr, so we can check this to be
    what is expected.

    23231c1e64
19. net: Use GetAdaptersAddresses to get local addresses on Windows

    Instead of a `gethostname` hack, use the official way of calling
    `GetAdaptersAddresses` to get local network addresses on Windows.
    
    As additional cleanup, move out `FromSockAddr` from MacOS and use it
    everywhere appropriate.
    
    Suggested by Ava Chow.

    219872fc75
20. laanwj force-pushed on Jan 14, 2025
21. 
    jonatack commented at 9:48 pm on January 15, 2025: member

    Concept ACK
22. in src/common/netif.cpp:298 in 219872fc75

    296+        if (status == ERROR_BUFFER_OVERFLOW && out_buf.size() < MAX_ADAPTER_ADDR_SIZE) {
    297+            // If status == ERROR_BUFFER_OVERFLOW, out_buf_len will contain the needed size.
    298+            // Unfortunately, this cannot be fully relied on, because another process may have added interfaces.
    299+            // So to avoid getting stuck due to a race condition, double the buffer size at least
    300+            // once before retrying (but only up to the maximum allowed size).
    301+            do {
    

    ---

    ---

    

    sipa commented at 5:58 pm on January 17, 2025:

    The loops seems unnecessary to me.

    0out_buf.resize(std::min(std::max(out_buf_len, out_buf.size() * 2), MAX_ADAPTER_ADDR_SIZE));
    

23. 
    sipa commented at 6:01 pm on January 17, 2025: member

    Concept and code review ACK 219872fc75e552c87c20b5c1ce7e15fa887eec20, but I am not familiar with the Windows network API, and have not tested it.
24. DrahtBot requested review from jonatack on Jan 17, 2025

Contributors
laanwj DrahtBot 1440000bytes jonatack sipa

Review Requested
jonatack

Labels
Windows P2P