#75: Implemented socket abstraction library by sgn4sangar · Pull Request #77 · exasol/udf-runner-cpp

sgn4sangar · 2026-09-30T07:16:45Z

fixes #75

sonarqubecloud · 2026-10-07T06:24:34Z

Quality Gate failed

Failed conditions
64.3% Coverage on New Code (required ≥ 80%)
1 New Code Smells (required ≤ 0)
D Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

tomuben · 2026-10-07T10:34:50Z

+    target_compatible_with = ["@platforms//os:linux"],
+)
+
+cc_fuzz_test(


There are the binary blobs (udf-runner-cpp/v2/fuzz/corpus/socket) but they are not used here!

tomuben · 2026-10-07T10:45:41Z

+#include <cstdint>
+#include <span>
+
+extern "C" int LLVMFuzzerTestOneInput(const std::uint8_t* data, const std::size_t size)


Add a description to this function as the behavior is not very obvious.
As far as I understand, if the written size is less than 4096, it does an early return (line 29), and does not execute the reader test code.

tomuben · 2026-10-07T10:46:49Z

+    std::array<int, 2> descriptors{};
+    if (::socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, descriptors.data()) != 0)
+    {
+        return 0;


Why is the treated as success? If the socketpair creation fails, shouln't this be treated as an error?

tomuben · 2026-10-07T10:47:41Z

Add doxygen comments to the public headers.

tomuben · 2026-10-07T10:50:46Z

+{
+    if (owned_fd < 0)
+    {
+        throw std::system_error(EBADF, std::generic_category(), "adopt invalid file descriptor");


I think std::invalid_argument fits better here

tomuben · 2026-10-07T10:52:46Z

+
+bool OwnedFileDescriptor::is_open() const noexcept
+{
+    return file_descriptor != -1;


According to the coding styles, we want to use this-> to access member variables.

tomuben · 2026-10-07T10:52:50Z

+
+int OwnedFileDescriptor::native_handle() const noexcept
+{
+    return file_descriptor;


According to the coding styles, we want to use this-> to access member variables.

tomuben · 2026-10-07T10:52:55Z

+    if (this != &other)
+    {
+        close();
+        file_descriptor = std::exchange(other.file_descriptor, -1);


According to the coding styles, we want to use this-> to access member variables.

tomuben · 2026-10-07T11:00:18Z

+    std::array<int, 2> first_pipe{};
+    std::array<int, 2> second_pipe{};
+    ASSERT_EQ(::pipe(first_pipe.data()), 0);
+    ASSERT_EQ(::pipe(second_pipe.data()), 0);
+    OwnedFileDescriptor source      = OwnedFileDescriptor::adopt_native_handle(first_pipe[0]);
+    OwnedFileDescriptor destination = OwnedFileDescriptor::adopt_native_handle(second_pipe[0]);
+    destination                     = std::move(source);
+    EXPECT_EQ(::close(second_pipe[0]), -1);
+    EXPECT_EQ(errno, EBADF);
+    ASSERT_EQ(::close(destination.release_native_handle()), 0);
+    ASSERT_EQ(::close(first_pipe[1]), 0);
+    ASSERT_EQ(::close(second_pipe[1]), 0);


Maybe check also that destination points to first_pipe[0] handle after the move operation

tomuben · 2026-10-07T11:02:00Z

+    std::array<int, 2> pipe_fds{};
+    ASSERT_EQ(::pipe(pipe_fds.data()), 0);
+    OwnedFileDescriptor descriptor = OwnedFileDescriptor::adopt_native_handle(pipe_fds[0]);
+    OwnedFileDescriptor moved(std::move(descriptor));


Maybe check also that handle of descriptor became invalid here

tomuben · 2026-10-07T11:08:30Z

+    ASSERT_EQ(::close(second_pipe[1]), 0);
+}
+
+TEST(SocketTest, ListenerConnectsAcceptsAndRequiresExplicitCleanup)


Maybe move all tests which use the unique_socket_path into a GTest fixture, which provides the temp path, and then automatically cleans up the temporary directory in TearDown().

You can also use the same class I created in the DB, see
https://github.com/exasol/db/blob/tu.master.SPOT-31038_refactor_udf_fw/Engine/src/exscript/pluggable/server_based/test_utils/temporary_directory_cleanup.h and https://github.com/exasol/db/blob/tu.master.SPOT-31038_refactor_udf_fw/Engine/src/exscript/pluggable/server_based/test_utils/temporary_directory_cleanup.cpp

and here is an example of usage:
https://github.com/exasol/db/blob/e3887e7a372074faada36afe74e1a7f3672899f6/Engine/src/exscript/pluggable/server_based/instance/connection/unix_domain_socket_transport_utest.cpp#L35

tomuben · 2026-10-07T11:10:21Z

Should we separate the unix-socket related tests from the socket-related tests, and have two test cc files?

tomuben · 2026-10-07T11:19:12Z

+    EXPECT_EQ(reader.read_some(eof_buffer), 0);
+}
+
+TEST(SocketTest, NonblockingReadReportsWouldBlock)


This test is a bit confusing. Maybe add an description

tomuben · 2026-10-07T11:25:41Z

+                throw std::system_error(EINVAL, std::generic_category(),
+                                        "I/O buffer size is too large");
+            }
+            total_size += std::size(buffer);


why std::size(buffer) and not buffer.size()?

tomuben · 2026-10-07T11:26:12Z

+            total_size += std::size(buffer);
+            // POSIX declares iovec::iov_base as void* even for sendmsg(), which does not mutate it.
+            iovecs.push_back(
+                {const_cast<std::byte*>(std::data(buffer)), std::size(buffer)}); // NOSONAR


why std::data(buffer) and not buffer.data()?

why std::size(buffer) and not buffer.size()?

tomuben · 2026-10-07T11:28:27Z

+    {
+        if (errno != EINTR)
+        {
+            throw std::system_error(errno, std::generic_category(), "connect Unix socket");


Is this really a system_error? if connect fails the given address could be wrong.

tomuben · 2026-10-07T11:30:49Z

+    OwnedFileDescriptor socket_fd = create_socket();
+    if (::bind(socket_fd.native_handle(), as_socket_address(address), address_length) == -1)
+    {
+        throw std::system_error(errno, std::generic_category(), "bind Unix socket listener");


same as above: If bind fails, the given address might be wrong

kratz00 · 2026-10-07T14:12:40Z

+    expect_system_error([path] { static_cast<void>(UnixSocketListener::bind(path)); },
+                        std::errc::address_in_use);
+    listener.close();
+    listener.unlink_path();


isn't that called in the destructor?

If the UDF Runner runs in the sandbox (current architecture) the database cleans-up the temp directory where the socket was created after the UDF Runner process died.
=> It's only necessary to clean up in the unit tests.

kratz00 · 2026-10-07T14:14:21Z

+
+UnixSocketListener::~UnixSocketListener()
+{
+    close();


I feel like the design does not follow RAII principles, we should also add the "unlink" to the destructor,
this would also thin out the public API

kratz00 · 2026-10-07T14:15:16Z

+    }
+    if (::listen(socket_fd.native_handle(), backlog) == -1)
+    {
+        throw std::system_error(errno, std::generic_category(), "listen on Unix socket");


bind created the path, listen fails, how do you unlink the path then?

kratz00 · 2026-10-07T14:19:34Z

+        return 0;
+    }
+    if (std::array<std::byte, 4096> received{};
+        reader.read_some(std::span(received).first(size)) != size)


I think it's a valid use case to read fewer bytes.

sgn4sangar requested a deployment to v2-fuzzing-pr-approval September 30, 2026 07:16 — with GitHub Actions Waiting

tomuben reviewed Sep 30, 2026

View reviewed changes

Comment thread udf-runner-cpp/v2/include/exasol/udf/v2/socket/socket.hpp Outdated

sgn4sangar added 8 commits October 3, 2026 13:07

#75: Add socket library API stubs

7a7177f

#75: Implement socket descriptor ownership

5f0b5d1

#75: Implement Unix socket connection and listener

871c26a

#75: Implement Unix socket stream operations

2634830

#75: Add Unix socket tests and fuzzing

5648402

#75: Group socket library files

c3bdcd3

#75: Fix socket clang-tidy findings

6ce6314

#75: Extend socket mutation coverage

f4ee0d7

sgn4sangar force-pushed the feature/75_implement_socket_abstraction_library branch from 7d4b2d4 to f4ee0d7 Compare October 3, 2026 07:42

sgn4sangar requested a deployment to v2-fuzzing-pr-approval October 3, 2026 07:42 — with GitHub Actions Waiting

sgn4sangar added 2 commits October 3, 2026 14:04

#75: Rename the development Lima template

c24a3cc

#75: Exclude socket tests from Mull

2590e45

sgn4sangar requested a deployment to v2-fuzzing-pr-approval October 5, 2026 09:27 — with GitHub Actions Waiting

#75: Address socket Sonar findings

cc4654e

sgn4sangar requested a deployment to v2-fuzzing-pr-approval October 5, 2026 11:39 — with GitHub Actions Waiting

#75: Format socket Sonar suppression

4873bc7

sgn4sangar requested a deployment to v2-fuzzing-pr-approval October 5, 2026 12:19 — with GitHub Actions Waiting

#75: Resolve remaining socket Sonar findings

25b581c

sgn4sangar requested a deployment to v2-fuzzing-pr-approval October 5, 2026 14:22 — with GitHub Actions Waiting

sgn4sangar added 2 commits October 5, 2026 19:57

#75: Move socket sources out of public headers

f4f46c8

#75: Group socket sources in a module folder

faa6427

sgn4sangar requested a deployment to v2-fuzzing-pr-approval October 6, 2026 05:00 — with GitHub Actions Waiting

sgn4sangar added 3 commits October 7, 2026 11:23

#75: Use short secure socket test paths

789c365

#75: Keep socket corpus changes untracked

9496da5

add more corpus files for test

706a115

sgn4sangar requested a deployment to v2-fuzzing-pr-approval October 7, 2026 06:11 — with GitHub Actions Waiting

tomuben reviewed Oct 7, 2026

View reviewed changes

kratz00 reviewed Oct 7, 2026

View reviewed changes

sgn4sangar added 3 commits October 8, 2026 13:57

#75: Split socket classes into public headers

ed77d36

unit tests for unix domain sockets

da8f14a

address comments

dc71239

sgn4sangar requested a deployment to v2-fuzzing-pr-approval October 9, 2026 11:18 — with GitHub Actions Waiting

fix formatting

4df8cec

sgn4sangar requested a deployment to v2-fuzzing-pr-approval October 9, 2026 16:22 — with GitHub Actions Waiting

Conversation

sgn4sangar commented Sep 30, 2026

Uh oh!

Uh oh!

sonarqubecloud Bot commented Oct 7, 2026

Quality Gate failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomuben commented Oct 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch is waiting to be deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants