tree 226532af656f0d5a88b339bcbf2678c8836ee2e2
parent 27877eb995a2da529b2baab3ae50eec785a37a99
author Hannu Lounento <hannu.lounento@vaisala.com> 1678698308 +0200
committer Patrick Williams <patrick@stwcx.xyz> 1683655710 +0000

Allow propagating exceptions from server methods

sdbusplus calls the D-Bus method implementation functions in such a way
that the callstack contains sd_bus C functions from libsystemd, and
sdbusplus only catches the specific exceptions that match the errors
defined for the D-Bus method before returning execution to the C
functions. If any D-Bus method implementation propagates any other
exception than the errors the method declares, the C functions in the
callstack may leak resources.

There isn't any documentation or other statement that would prohibit
exceptions from being thrown / propagated from method implementations.
This seems to be a known issue though [1]:

    By throwing an exception, you're not making an error return to the
    calling client, but instead blowing through all of the `sd_bus` C
    code with your C++ exception and putting your application into an
    invalid state. At a minimum you are leaking memory.

Thus, catch any exceptions propagated from method implementations and
re-throw them from sdbusplus::bus::bus::process*().

Catching and re-throwing avoids resource leaks from sd_bus C functions.
And as a consequence it would allow propagating exceptions in the normal
way until the caller that is prepared to handle them -- ultimately even
up to main(). Propagating exceptions to `main()` allows for terminating
the process in a controlled fashion in case unexpected failures, which,
in turn, allows for a chance to recover from potential failures modes
that persist  until application restart (e.g. due to some invalid state
stored in RAM).

Also, terminating the application, in which case the D-Bus daemon
returns the standard error org.freedesktop.DBus.Error.NoReply to the
client, avoids the need to declare an error for internal failures in the
D-Bus API. Internal errors in API add little value over
org.freedesktop.DBus.Error.NoReply and bloat the API as clients many
times can't handle them in any better way than
org.freedesktop.DBus.Error.NoReply.

Only std::exceptions are catched (i.e. not `catch (...)`) and propagated
to keep the implementation simple. The assumption is that only
exceptions (i.e.  classes derived from std::exception) are used for
reporting errors.

Tested: With the calculator example changed to throw an unexpected exception:

    From 914e045d1f92a1730d32f84ac7a74cd867c76760 Mon Sep 17 00:00:00 2001
    From: Hannu Lounento <hannu.lounento@vaisala.com>
    Date: Mon, 13 Mar 2023 15:53:17 +0200
    Subject: [PATCH] [dbg] Add a propagating exception to the calculator example

    Not to be merged to sdbusplus master but only for RFC / demonstration
    purposes.

    Can be triggered by commands like

        busctl --user call net.poettering.Calculator /net/poettering/calculator net.poettering.Calculator Clear
        busctl --user get-property net.poettering.Calculator /net/poettering/calculator net.poettering.Calculator LastResult
        busctl --user set-property net.poettering.Calculator /net/poettering/calculator net.poettering.Calculator LastResult x 1

    Change-Id: Ie9f47227afe7e0fa9765c602bec987aea71b9b86
    Signed-off-by: Hannu Lounento <hannu.lounento@vaisala.com>
    ---
    example/calculator-server.cpp | 31 ++++++++++++++++++++++++++-----
    1 file changed, 26 insertions(+), 5 deletions(-)

    diff --git a/example/calculator-server.cpp b/example/calculator-server.cpp
    index aa60662..eba9cc0 100644
    --- a/example/calculator-server.cpp
    +++ b/example/calculator-server.cpp
    @@ -3,6 +3,7 @@
    #include <net/poettering/Calculator/server.hpp>
    #include <sdbusplus/server.hpp>

    +#include <cstdlib>
    #include <iostream>
    #include <string_view>

    @@ -42,10 +43,22 @@ struct Calculator : Calculator_inherit
        /** Clear lastResult, broadcast 'Cleared' signal */
        void clear() override
        {
    -        auto v = lastResult();
    -        lastResult(0);
    -        cleared(v);
    -        return;
    +        throw std::runtime_error{"A propagating exception"};
    +    }
    +
    +    int64_t lastResult() const override
    +    {
    +        throw std::runtime_error{"A propagating exception"};
    +    }
    +
    +    int64_t lastResult(int64_t /*value*/, bool /*skipSignal*/) override
    +    {
    +        throw std::runtime_error{"A propagating exception"};
    +    }
    +
    +    int64_t lastResult(int64_t /*value*/) override
    +    {
    +        throw std::runtime_error{"A propagating exception"};
        }
    };

    @@ -71,5 +84,13 @@ int main()
        Calculator c1{b, path};

        // Handle dbus processing forever.
    -    b.process_loop();
    +    try
    +    {
    +        b.process_loop();
    +    }
    +    catch (const std::exception& e)
    +    {
    +        std::cerr << "Terminating due to a fatal condition: " << e.what() << std::endl;
    +        return EXIT_FAILURE;
    +    }
    }
    --
    2.40.0

1) Built and executed unit tests:

    sdbusplus$ rm -rf build/ && CXXFLAGS=-Wno-maybe-uninitialized meson setup build && cd build && ninja && ninja test
    The Meson build system
    Version: 0.62.2
    Source dir: /path/to/sdbusplus
    Build dir: /path/to/sdbusplus/build
    Build type: native build
    Project name: sdbusplus
    Project version: 1.0.0
    C compiler for the host machine: ccache cc (gcc 12.2.1 "cc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4)")
    C linker for the host machine: cc ld.bfd 2.37-37
    C++ compiler for the host machine: ccache c++ (gcc 12.2.1 "c++ (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4)")
    C++ linker for the host machine: c++ ld.bfd 2.37-37
    Host machine cpu family: x86_64
    Host machine cpu: x86_64
    Found pkg-config: /usr/bin/pkg-config (1.8.0)
    Run-time dependency libsystemd found: YES 250
    Program python3 (inflection, yaml, mako) found: YES (/usr/bin/python3) modules: inflection, yaml, mako
    Run-time dependency Boost found: YES 1.76.0 (/usr)
    Program sdbus++ found: YES (/path/to/sdbusplus/tools/sdbus++)
    Program sdbus++ found: YES (overridden)
    Program sdbus++-gen-meson found: YES (/path/to/sdbusplus/tools/sdbus++-gen-meson)
    Program sdbus++-gen-meson found: YES (overridden)
    Header <boost/asio.hpp> has symbol "boost::asio::io_context" : YES
    Run-time dependency Boost (found: context, coroutine) found: YES 1.76.0 (/usr)
    Run-time dependency GTest found: YES 1.11.0
    Run-time dependency GMock found: YES 1.11.0
    Build targets in project: 34

    Found ninja-1.10.2 at /usr/bin/ninja
    [77/77] Linking target example/calculator-client
    [0/1] Running all tests.
    1/21 test_async_task                     OK              0.03s
    2/21 test_bus_list_names                 OK              0.02s
    3/21 test_bus_match                      OK              0.02s
    4/21 test_bus_exception                  OK              0.02s
    5/21 test_exception_sdbus_error          OK              0.02s
    6/21 test_message_append                 OK              0.01s
    7/21 test_message_native_types           OK              0.01s
    8/21 test_message_read                   OK              0.03s
    9/21 test_message_types                  OK              0.02s
    10/21 test_unpack_properties              OK              0.02s
    11/21 test_utility_tuple_to_array         OK              0.02s
    12/21 test_utility_type_traits            OK              0.01s
    13/21 test-bus_aio                        OK              0.01s
    14/21 test-vtable                         OK              0.01s
    15/21 test-server                         OK              0.01s
    16/21 test-server-message-variant         OK              0.01s
    17/21 test_message_call                   OK              0.07s
    18/21 test_event_event                    OK              0.31s
    19/21 test_async_timer                    OK              0.51s
    20/21 test_async_context                  OK              0.52s
    21/21 test_timer                          OK             18.02s

    Ok:                 21
    Expected Fail:      0
    Fail:               0
    Unexpected Pass:    0
    Skipped:            0
    Timeout:            0

    [...]

-Wno-maybe-uninitialized was used because the current master
384943be7e81b08ed102abfaa3f2be2ad915e800 ("sdbus++: async: client: fix
client_t usage") failed to build with

    In file included from /usr/include/gtest/internal/gtest-death-test-internal.h:39,
                    from /usr/include/gtest/gtest-death-test.h:41,
                    from /usr/include/gtest/gtest.h:64,
                    from /usr/include/gmock/internal/gmock-internal-utils.h:47,
                    from /usr/include/gmock/gmock-actions.h:145,
                    from /usr/include/gmock/gmock.h:59,
                    from ../include/sdbusplus/test/sdbus_mock.hpp:6,
                    from ../test/exception/sdbus_error.cpp:4:
    In copy constructor ‘testing::internal::MatcherBase<T>::MatcherBase(const testing::internal::MatcherBase<T>&) [with T = sd_bus_error*]’,
        inlined from ‘testing::Matcher<sd_bus_error*>::Matcher(const testing::Matcher<sd_bus_error*>&)’ at /usr/include/gtest/gtest-matchers.h:479:7,
        inlined from ‘testing::internal::MockSpec<int(sd_bus_error*, int)> sdbusplus::SdBusMock::gmock_sd_bus_error_set_errno(const testing::Matcher<sd_bus_error*>&, const testing::Matcher<int>&)’ at ../include/sdbusplus/test/sdbus_mock.hpp:51:5,
        inlined from ‘virtual void {anonymous}::SdBusError_NotSetErrno_Test::TestBody()’ at ../test/exception/sdbus_error.cpp:59:5:
    /usr/include/gtest/gtest-matchers.h:302:33: error: ‘<unnamed>.testing::Matcher<sd_bus_error*>::<unnamed>.testing::internal::MatcherBase<sd_bus_error*>::buffer_’ may be used uninitialized [-Werror=maybe-uninitialized]
    302 |       : vtable_(other.vtable_), buffer_(other.buffer_) {
        |                                 ^~~~~~~~~~~~~~~~~~~~~~

on Fedora 36 with

    $ gcc --version
    gcc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4)

and

    $ dnf info gtest-devel
    [...]
    Name         : gtest-devel
    Version      : 1.11.0
    Release      : 2.fc36
    Architecture : x86_64

2) Verified no memory leaks were reported by Valgrind anymore:

    sdbusplus/build$ valgrind --leak-check=full --show-leak-kinds=all ./example/calculator-server
    ==61886== Memcheck, a memory error detector
    ==61886== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
    ==61886== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
    ==61886== Command: ./example/calculator-server
    ==61886==

and called the throwing method:

    build$ busctl --user call net.poettering.Calculator /net/poettering/calculator net.poettering.Calculator Clear
    Call failed: Remote peer disconnected

and verified the process terminates but produces no memory leaks:

    Terminating due to a fatal condition: A propagating exception
    ==61886==
    ==61886== HEAP SUMMARY:
    ==61886==     in use at exit: 0 bytes in 0 blocks
    ==61886==   total heap usage: 143 allocs, 143 frees, 96,732 bytes allocated
    ==61886==
    ==61886== All heap blocks were freed -- no leaks are possible
    ==61886==
    ==61886== For lists of detected and suppressed errors, rerun with: -s
    ==61886== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Similarly reading the `LastResult` property:

    build$ valgrind --leak-check=full --show-leak-kinds=all ./example/calculator-server
    ==36140== Memcheck, a memory error detector
    ==36140== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
    ==36140== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
    ==36140== Command: ./example/calculator-server
    ==36140==

    build$ busctl --user get-property net.poettering.Calculator /net/poettering/calculator net.poettering.Calculator LastResult
    Failed to get property LastResult on interface net.poettering.Calculator: Remote peer disconnected

    Terminating due to a fatal condition: A propagating exception
    ==36140==
    ==36140== HEAP SUMMARY:
    ==36140==     in use at exit: 0 bytes in 0 blocks
    ==36140==   total heap usage: 148 allocs, 148 frees, 96,992 bytes allocated
    ==36140==
    ==36140== All heap blocks were freed -- no leaks are possible
    ==36140==
    ==36140== For lists of detected and suppressed errors, rerun with: -s
    ==36140== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

and writing the property:

    ==36383== Memcheck, a memory error detector
    ==36383== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
    ==36383== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
    ==36383== Command: ./example/calculator-server
    ==36383==

    build$ busctl --user set-property net.poettering.Calculator /net/poettering/calculator net.poettering.Calculator LastResult x 1

    Terminating due to a fatal condition: A propagating exception
    ==36383==
    ==36383== HEAP SUMMARY:
    ==36383==     in use at exit: 0 bytes in 0 blocks
    ==36383==   total heap usage: 146 allocs, 146 frees, 97,010 bytes allocated
    ==36383==
    ==36383== All heap blocks were freed -- no leaks are possible
    ==36383==
    ==36383== For lists of detected and suppressed errors, rerun with: -s
    ==36383== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

[1] https://lore.kernel.org/openbmc/YTDvfIn4Z05mGdCx@heinlein/

Change-Id: I014099a91f526a92c0f6646e75b4029513ad19a8
Signed-off-by: Hannu Lounento <hannu.lounento@vaisala.com>