Consolidation and Improvement of Boost.Asio Socket Layer

[KaganCanSit]

Dear Botan Development Team,

In reference to issue #4487, I have undertaken the consolidation and enhancement of the Boost Asio socket layer utilized within the library. This initiative aims to promote a more streamlined and maintainable architecture by refactoring redundant code in socket.cpp and socket_udp.cpp, thereby improving the readability of the Asio-related components.​

To achieve this, I have organized the Boost Asio components into a separate header file and implemented them as a template class. Given that this is not an operating system-based socket management, I deemed it inappropriate to include these in the socket_platform.h file, as was done in my previous pull request awaiting review (PR #4660).​

I have created templates for both UDP and TCP, leveraging the if constexpr feature introduced in C++17 to handle differing sections. While this approach may initially seem complex, I believe it allows for more manageable code from a single point.​

After implementing these changes, I referred to the Boost documentation to ensure alignment with best practices. Although Boost is not a library I frequently use, I aimed to make minimal changes to facilitate an easily reviewable commit. A thorough review the boost functions would be beneficial.​

The commands I executed for building and testing are as follows:

python3 src/scripts/dev_tools/run_clang_format.py --clang-format=clang-format-17 --src-dir=src

sudo apt-get update && sudo apt-get install libboost-all-dev

ninja clean

./configure.py --without-documentation --with-boost -cc=clang --compiler-cache=ccache --build-targets=static,cli,tests --build-tool=ninja

ninja

./botan-test --test-threads=1 --run-long-tests

python3 src/scripts/test_cli.py ./botan cli_tls_socket_tests

Post-refactoring, the codebase compiles successfully, and the botan-test suite runs without errors. However, I am not entirely certain about the comprehensiveness of the tests. I would greatly appreciate guidance and assistance in conducting more effective testing.​

I appreciate the freedom the Botan project provides in developing code based on current standards. However, these are not always the standards I adhere to in my professional life.​ I acknowledge that there may be errors in my contributions and am open to making necessary changes. I welcome any feedback and am ready to collaborate to ensure the highest code quality.​

Important Additional Note: During these modifications, I observed that the m_time parameter exhibits different time parameter types across socket implementations. To minimize changes, I refrained from altering each file individually. However, I believe this should be addressed to avoid confusion and buggy code.

Note: I believe that the content of my pending PR #4660 should be merged prior to this request. A minor rebase might be necessary.​

Thank you for considering my contribution.

Best regards,

[coveralls]

coverage: 90.563% (+0.003%) from 90.56%
when pulling 73021c5 on KaganCanSit:feature-cleanup-asio-code
into c80dbf8 on randombit:master.

[KaganCanSit]

Hmm,

Some checks failed
1 failed, 40 passed

src/lib/utils/socket/socket_udp.cpp checked
src/lib/utils/socket/socket_udp.cpp:75:21: warning: unnecessary explicit cast to same type as subexpression (i.e. 'unsigned int'), remove this cast [readability-redundant-casting]
75 | salen = static_cast<socklen_t>(res->ai_addrlen);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~
/usr/include/netdb.h:571:13: note: source type is derived from referencing this member
571 | socklen_t ai_addrlen; /* Length of socket address. */
| ~~~~~~~~~ ^

checked src/lib/utils/socket/socket.cpp
2 files checked in 29 seconds
1 file found clang-tidy errors

Only this check failed. But I don't think I did anything that would directly affect this. It's a bit strange. I think I can solve it as follows, but I'll take a look at it in a while.

Remove

salen = static_cast<socklen_t>(res->ai_addrlen);

Update

salen = res->ai_addrlen;

[reneme]

Only this check failed. But I don't think I did anything that would directly affect this. It's a bit strange. I think I can solve it as follows, but I'll take a look at it in a while.

This seems to be an artifact of our clang-tidy setup where it is run after configuring --with-boost. This used to disable the relevant code path and the warning got missed. Now that you are restructuring things, the path is hit and clang-tidy finds the code smell.

Your proposed fix seems reasonable to me, though.

[KaganCanSit]

Thank you for your time. I looked into this situation afterwards, but forgot to write it.

The type of res->ai_addrlen from getaddrinfo differs depending on the operating system:

POSIX (Linux/macOS): res->ai_addrlen type: socklen_t
Windows: res->ai_addrlen type: int

If the conversion is unconditionally removed, I believe the code may fail static analysis on Windows. Since this value is passed to the ::sendto function

botan/src/lib/utils/socket/socket_udp.cpp

Line 112 in fe5e2d6

socket_op_ret_type sent = ::sendto(m_socket,

which expects a socklen_t parameter a cast is required on Windows where the original type is int.

To ensure both portability and readability, I propose handling the cast conditionally based on the platform, like this:

#if defined(BOTAN_TARGET_OS_HAS_WINSOCK2)
            salen = static_cast<socklen_t>(res->ai_addrlen);
#else
            salen = res->ai_addrlen;
#endif

Does this approach look acceptable to you? Little bit noisy, I am not sure.

--
I'm going ahead with this change for now to see how it behaves. Of course, we can adjust it later based on your feedback if needed.

[reneme]

The type of res->ai_addrlen from getaddrinfo differs depending on the operating system

Well, that's unfortunate. Thanks for looking into it.

Does this approach look acceptable to you? Little bit noisy, I am not sure.

Alternatively, you could just disable this clang-tidy warning. It's after all not harmful to do the cast in every environment:

salen = static_cast<socklen_t>(res->ai_addrlen);  // NOLINT(*-readability-redundant-casting)

[KaganCanSit]

My thoughts were also in this direction for adaptability and readability. However, I did not want to edit it in a way that would ignore a finding that occurred as a result of static analysis without consulting you. I am updating it as you suggested.

[KaganCanSit]

The conflict src/lib/utils/socket/socket_udp.cpp was resolved by re-basing the branch with the master branch.

[KaganCanSit]

Similar to #4660, more than a year has passed since the improvements made here. Most of the files contain changes, however minor. I believe this improvement needs to be revisited comprehensively, so I’m closing it for now.