From a2eab1185ade975d743b3db9c74337eab49c2507 Mon Sep 17 00:00:00 2001 From: Khang D Nguyen Date: Mon, 2 Jun 2025 09:54:19 +0700 Subject: [PATCH 1/3] mctpd: zero initialize link data On non-bus-owner modes, link->slot_busowner will be uninitialized, leading to segmentation fault when running sd_bus_slot_unref. ==3850684== Conditional jump or move depends on uninitialised value(s) ==3850684== at 0x48FB448: sd_bus_slot_unref (in /usr/lib/aarch64-linux-gnu/libsystemd.so.0.38.0) ==3850684== by 0x1123E3: free_link (mctpd.c:3263) ==3850684== by 0x1125CB: free_links (mctpd.c:3308) ==3850684== by 0x113D63: main (mctpd.c:3900) Simply zero initialize the link data. Signed-off-by: Khang D Nguyen --- src/mctpd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mctpd.c b/src/mctpd.c index 3c798ace..8d174509 100644 --- a/src/mctpd.c +++ b/src/mctpd.c @@ -3573,7 +3573,7 @@ static int add_interface(struct ctx *ctx, int ifindex) return -ENOENT; } - struct link *link = malloc(sizeof(*link)); + struct link *link = calloc(1, sizeof(*link)); if (!link) return -ENOMEM; From 7ca231dbc9195c5fbf5b6f9490c7165dae18cd5d Mon Sep 17 00:00:00 2001 From: Khang D Nguyen Date: Tue, 3 Jun 2025 12:17:41 +0700 Subject: [PATCH 2/3] mctpd: free config file path on exit ASAN complained about this config file path memory leak. Free it on program exit. Also, there is a clangd warning on implicitly freeing const char*. Instead of casting to void* explicitly, I changed it to be char* to reflect the fact that we own the string allocation better. Signed-off-by: Khang D Nguyen --- src/mctpd.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/mctpd.c b/src/mctpd.c index 8d174509..8234fb4e 100644 --- a/src/mctpd.c +++ b/src/mctpd.c @@ -196,7 +196,7 @@ struct ctx { sd_bus *bus; // Configuration - const char *config_filename; + char *config_filename; mctp_nl *nl; @@ -3829,6 +3829,11 @@ static void setup_config_defaults(struct ctx *ctx) ctx->default_role = ENDPOINT_ROLE_BUS_OWNER; } +static void free_config(struct ctx *ctx) +{ + free(ctx->config_filename); +} + int main(int argc, char **argv) { struct ctx ctxi = {0}, *ctx = &ctxi; @@ -3900,6 +3905,7 @@ int main(int argc, char **argv) free_links(ctx); free_peers(ctx); free_nets(ctx); + free_config(ctx); mctp_nl_close(ctx->nl); From b2463e1273fc736b07597c79d1b8b8b8050b1009 Mon Sep 17 00:00:00 2001 From: Khang D Nguyen Date: Mon, 2 Jun 2025 09:54:19 +0700 Subject: [PATCH 3/3] tests: allow passing config as a fixture mctpd now requests a config fixture, which is None by default to ensure previous behaviour. Test authors can now override the config using pytest fixtures overriding facility. An example on overriding per file basis is included in the commit. Signed-off-by: Khang D Nguyen --- tests/conftest.py | 8 ++++++-- tests/mctp_test_utils.py | 7 +++++++ tests/mctpd/__init__.py | 19 +++++++++++++++++-- tests/test_mctpd_endpoint.py | 15 +++++++++++++++ 4 files changed, 45 insertions(+), 4 deletions(-) create mode 100644 tests/test_mctpd_endpoint.py diff --git a/tests/conftest.py b/tests/conftest.py index d18772a9..e1135f73 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -21,8 +21,12 @@ async def dbus(): yield bus @pytest.fixture -async def mctpd(nursery, dbus, sysnet): - m = fake_mctpd.MctpdWrapper(dbus, sysnet) +def config(): + return None + +@pytest.fixture +async def mctpd(nursery, dbus, sysnet, config): + m = fake_mctpd.MctpdWrapper(dbus, sysnet, config = config) await m.start_mctpd(nursery) yield m res = await m.stop_mctpd() diff --git a/tests/mctp_test_utils.py b/tests/mctp_test_utils.py index 6f23a026..b0de2fe9 100644 --- a/tests/mctp_test_utils.py +++ b/tests/mctp_test_utils.py @@ -6,6 +6,13 @@ async def mctpd_mctp_iface_obj(dbus, iface): ) return await obj.get_interface('au.com.codeconstruct.MCTP.BusOwner1') +async def mctpd_mctp_iface_control_obj(dbus, iface): + obj = await dbus.get_proxy_object( + "au.com.codeconstruct.MCTP1", + "/au/com/codeconstruct/mctp1/interfaces/" + iface.name, + ) + return await obj.get_interface("au.com.codeconstruct.MCTP.Interface1") + async def mctpd_mctp_endpoint_obj(dbus, path, iface): obj = await dbus.get_proxy_object( 'au.com.codeconstruct.MCTP1', diff --git a/tests/mctpd/__init__.py b/tests/mctpd/__init__.py index e9eeef65..67469cc7 100644 --- a/tests/mctpd/__init__.py +++ b/tests/mctpd/__init__.py @@ -6,6 +6,7 @@ import socket import struct import sys +import tempfile import trio import uuid @@ -975,11 +976,12 @@ async def send_fd(sock, fd): class MctpdWrapper: - def __init__(self, bus, sysnet, binary=None): + def __init__(self, bus, sysnet, binary=None, config=None): self.bus = bus self.system = sysnet.system self.network = sysnet.network self.binary = binary or './test-mctpd' + self.config = config (self.sock_local, self.sock_remote) = self.socketpair() def socketpair(self): @@ -1056,8 +1058,18 @@ def name_owner_changed(name, new_owner, old_owner): # start mctpd, passing our control socket env = os.environ.copy() env['MCTP_TEST_SOCK'] = str(self.sock_remote.fileno()) + + if self.config: + config_file = tempfile.NamedTemporaryFile('w', prefix="mctp.conf.") + config_file.write(self.config) + config_file.flush() + command = [self.binary, '-v', '-c', config_file.name] + else: + config_file = None + command = [self.binary, '-v'] + proc = await trio.lowlevel.open_process( - [self.binary, '-v'], # todo: flexible paths + command = command, pass_fds = (1, 2, self.sock_remote.fileno()), env = env, ) @@ -1074,6 +1086,9 @@ def name_owner_changed(name, new_owner, old_owner): proc_rc = await proc.wait() + if config_file: + config_file.close() + await send_chan.send(proc_rc) Sysnet = namedtuple('SysNet', ['system', 'network']) diff --git a/tests/test_mctpd_endpoint.py b/tests/test_mctpd_endpoint.py new file mode 100644 index 00000000..49e8f0c7 --- /dev/null +++ b/tests/test_mctpd_endpoint.py @@ -0,0 +1,15 @@ +import pytest +from mctp_test_utils import * +from mctpd import * + +@pytest.fixture(name="config") +def endpoint_config(): + return """ + mode = "endpoint" + """ + +""" Test if mctpd is running as an endpoint """ +async def test_endpoint_role(dbus, mctpd): + obj = await mctpd_mctp_iface_control_obj(dbus, mctpd.system.interfaces[0]) + role = await obj.get_role() + assert str(role) == "Endpoint"