Use RAII for addrinfo lifetime management

[KaganCanSit]

Hello,
It has been over a year since my PR #4660, and I honestly don't remember many of the details. While revisiting that branch, I identified changes that could independently contribute to the project and decided to submit them as separate PRs.

This PR applies RAII-based lifetime management to addrinfo resources returned by getaddrinfo(). In the current codebase, ::freeaddrinfo() is called manually, which can leak if an exception is thrown before the cleanup call is reached.

The changes are based on the approach discussed with @reneme in this review comment (Since it is marked as resolved, you cannot directly move to the comment.): a std::unique_ptr with a stateless lambda deleter combined with Botan::out_ptr() from stl_util.h.

Quotes:
@KaganCanSit

The freeaddrinfo() function is defined to free one or more addrinfo structures returned by getaddrinfo() and the additional memory associated with them. It usually frees the resulting linked list (addrinfo chain) when getaddrinfo() succeeds. The POSIX standard assumes that the freeaddrinfo() call will only apply to a valid addrinfo list returned by getaddrinfo(). The text of the standard does not explicitly define the case where an invalid pointer (e.g. NULL) is passed to freeaddrinfo() – this case is left undefined.

So I'm a bit hesitant here. I thought it might be UB if the addrinfo retrieval fails and is freed. However, I liked the approach you suggested in other comments. I learned, thanks. I'll reconsider in the last case.

@reneme

Mhm, you might have a point here. It certainly doesn't hurt to check the pointer for nullptr before calling freeaddrinfo(). When it comes to C-APIs I've seen both: some are fine with receiving a nullptr others aren't.

Instead of writing this out multiple times, you could simply define a typedef in the new socket_platform.h and use that in tls_client.cpp and the socket*.cpps.

Like so:

using unique_addrinfo_ptr = std::unique_ptr<addrinfo, decltype([](addrinfo* p) {
if(p != nullptr) {
::freeaddrinfo(p);
}
})>;
// in the *.cpp files
Botan::OS::Socket_Platform::unique_addrinfo_ptr res = nullptr;

The using declarations are defined as private members in each class since there is currently no shared internal header across socket files. (socket_utils.h exists but is external and not included, I think there is a reason for this.)

I edited the bogo_shim.cpp file to ensure full compatibility with the others and to allow direct intervention when Botan::out_ptr is removed. However, if I am mistaken, please let me know.

Compile and Test

./configure.py --without-documentation --compiler-cache=ccache --build-targets=static,cli,tests && 
make clean && make -j$(nproc) && 
python3 src/scripts/test_cli.py ./botan cli_tls

I am happy to make further adjustments based on your feedback.
Best regards.

[coveralls]

coverage: 90.32% (-0.001%) from 90.321%
when pulling 11695c8 on KaganCanSit:fix/use-RAII-for-addrinfo-lifetime
into fac2691 on randombit:master.

[KaganCanSit]

If this development is deemed appropriate, the following simplification can through a separate branch.

diff --git a/src/cli/tls_client.cpp b/src/cli/tls_client.cpp
index 56cc4a76a..f18f106ef 100644
--- a/src/cli/tls_client.cpp
+++ b/src/cli/tls_client.cpp
@@ -397,11 +397,8 @@ class TLS_Client final : public Command {
             throw CLI_Error("getaddrinfo failed for " + host);
          }
 
-         socket_type fd = 0;
-         bool success = false;
-
          for(const addrinfo* rp = res.get(); rp != nullptr; rp = rp->ai_next) {
-            fd = ::socket(rp->ai_family, rp->ai_socktype, rp->ai_protocol);
+            socket_type fd = ::socket(rp->ai_family, rp->ai_socktype, rp->ai_protocol);
 
             if(fd == invalid_socket()) {
                continue;
@@ -412,16 +409,10 @@ class TLS_Client final : public Command {
                continue;
             }
 
-            success = true;
-            break;
-         }
-
-         if(!success) {
-            // no address succeeded
-            throw CLI_Error("Connecting to host failed");
+            return fd;
          }
-
-         return fd;
+         // no address succeeded
+         throw CLI_Error("Connecting to host failed");
       }

[reneme]

Thanks for taking this out of #4660. Looks good to me.

[randombit]

I'm a little skeptical of the nullptr checks to freeaddrinfo in that the most recent references I could find to any platforms actually having a problem with this are FreeBSD circa 2004 and Android circa 2014, and

Patches that complicate the code in order to support obsolete operating systems will likely be rejected.

That said it's a pretty minor complication and indeed - bizarrely IMO - latest SUS does not explicitly allow NULL to be passed to freeaddrinfo, though IMO this wording (emphasis mine) allows it

The freeaddrinfo() function shall support the freeing of arbitrary sublists of an addrinfo list originally returned by getaddrinfo().

Since how do you represent an arbitrary sublist of length zero but with a nullptr?

[KaganCanSit]

Patches that complicate the code in order to support obsolete operating systems will likely be rejected.

I completely agree with this principle. In business life, I see that maintaining dependency on old platforms significantly increases maintenance costs in the long run.

Actually, my assessment here is largely in line with yours. Since addrinfo structures are stored as a linked list, a structure with a length of zero is needed to determine the end of the list (NULL / nullptr). A typical freeaddrinfo implementation also frees the chain as follows:

while (ai) {
   next = ai->ai_next;
   free(ai);
   ai = next;
}

During development and look again today, what particularly caught my attention was that some examples did not allow early termination with a call to freeaddrinfo(NULL), and the standard did not explicitly state this behavior as it does for free(NULL).

https://linux.die.net/man/3/freeaddrinfo

struct addrinfo hints;

	// ...

   s = getaddrinfo(NULL, argv[1], &hints, &result);
    if (s != 0) {
        fprintf(stderr, "getaddrinfo: %s\n", gai_strerror(s));
        exit(EXIT_FAILURE);
    }

   /* getaddrinfo() returns a list of address structures.
       Try each address until we successfully bind(2).
       If socket(2) (or bind(2)) fails, we (close the socket
       and) try the next address. */

   for (rp = result; rp != NULL; rp = rp->ai_next) {
        sfd = socket(rp->ai_family, rp->ai_socktype,
                rp->ai_protocol);
        if (sfd == -1)
            continue;

       if (bind(sfd, rp->ai_addr, rp->ai_addrlen) == 0)
            break;                  /* Success */

       close(sfd);
    }

   if (rp == NULL) {               /* No address succeeded */
        fprintf(stderr, "Could not bind\n");
        exit(EXIT_FAILURE);
    }

   freeaddrinfo(result);           /* No longer needed */

I also saw this issue specifically addressed in the following projects (2018, 2019, 2023):

• https://patchwork.openvpn.net/project/openvpn2/patch/20230209153658.24001-1-ynezz@true.cz/#5791
• Random process crashes on the (almost) lastest portable OpenSMTPD/OpenSMTPD#929
• https://git.openembedded.org/openembedded-core-contrib/tree/meta/recipes-connectivity/nfs-utils/nfs-utils/0001-Do-not-pass-null-pointer-to-freeaddrinfo.patch?h=paule/distrodata-selftest-fix

As you mentioned, it has a very small cost (a single if). However, I am aware that this could be an overly protective approach.

I am considering addressing the above comment as a separate PR. If you prefer, we can remove this change or leave it as is.

Thank you for your time, review, and sharing your knowledge.

Best regards.