From 14533598bd0eb9f1f5f563e80fbd9645638831f6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 7 Jan 2025 14:41:22 +0100 Subject: [PATCH 1/3] sd-json: make it safe to call sd_json_dispatch_full() with a NULL table This is useful for generating good errors when dispatching varlink methods that take no parameters, as we'll still generate precise errors in that case, taking a NULL table as equivalent as one with no entries. --- src/libsystemd/sd-json/sd-json.c | 8 ++++---- src/libsystemd/sd-varlink/sd-varlink.c | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/libsystemd/sd-json/sd-json.c b/src/libsystemd/sd-json/sd-json.c index b8633548c8..99d941e1ef 100644 --- a/src/libsystemd/sd-json/sd-json.c +++ b/src/libsystemd/sd-json/sd-json.c @@ -5104,7 +5104,7 @@ _public_ int sd_json_dispatch_full( } m = 0; - for (const sd_json_dispatch_field *p = table; p->name; p++) + for (const sd_json_dispatch_field *p = table; p && p->name; p++) m++; found = newa0(bool, m); @@ -5117,12 +5117,12 @@ _public_ int sd_json_dispatch_full( assert_se(key = sd_json_variant_by_index(v, i)); assert_se(value = sd_json_variant_by_index(v, i+1)); - for (p = table; p->name; p++) + for (p = table; p && p->name; p++) if (p->name == POINTER_MAX || streq_ptr(sd_json_variant_string(key), p->name)) break; - if (p->name) { /* Found a matching entry! 🙂 */ + if (p && p->name) { /* Found a matching entry! 🙂 */ sd_json_dispatch_flags_t merged_flags; merged_flags = flags | p->flags; @@ -5223,7 +5223,7 @@ _public_ int sd_json_dispatch_full( } } - for (const sd_json_dispatch_field *p = table; p->name; p++) { + for (const sd_json_dispatch_field *p = table; p && p->name; p++) { sd_json_dispatch_flags_t merged_flags = p->flags | flags; if ((merged_flags & SD_JSON_MANDATORY) && !found[p-table]) { diff --git a/src/libsystemd/sd-varlink/sd-varlink.c b/src/libsystemd/sd-varlink/sd-varlink.c index 9e8f2a66f8..962adcb869 100644 --- a/src/libsystemd/sd-varlink/sd-varlink.c +++ b/src/libsystemd/sd-varlink/sd-varlink.c @@ -2755,7 +2755,6 @@ _public_ int sd_varlink_dispatch(sd_varlink *v, sd_json_variant *parameters, con int r; assert_return(v, -EINVAL); - assert_return(table, -EINVAL); /* A wrapper around json_dispatch_full() that returns a nice InvalidParameter error if we hit a problem with some field. */ From f249cc76180864ca95ba7ca0c48d6389c47e84d5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 7 Jan 2025 18:51:10 +0100 Subject: [PATCH 2/3] varlink: tweak what we include in "system error" messages We so far only included the numeric Linux errno. That's pretty Linux specific however. Hence, let's improve things and include an origin string, that clearly marks Linux as origin. Also, include the string name of the error. Take these two fields into account when translating back, too. So that we prefer going by symbolic name rather than by numeric id. --- src/libsystemd/sd-varlink/sd-varlink.c | 61 +++++++++++++++---- .../sd-varlink/varlink-io.systemd.c | 11 +++- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/src/libsystemd/sd-varlink/sd-varlink.c b/src/libsystemd/sd-varlink/sd-varlink.c index 962adcb869..8658c81f67 100644 --- a/src/libsystemd/sd-varlink/sd-varlink.c +++ b/src/libsystemd/sd-varlink/sd-varlink.c @@ -2678,10 +2678,21 @@ _public_ int sd_varlink_error_invalid_parameter_name(sd_varlink *v, const char * } _public_ int sd_varlink_error_errno(sd_varlink *v, int error) { + + /* This generates a system error return that includes the Linux error number, and error name. The + * error number is kinda Linux specific (and to some degree the error name too), hence let's indicate + * the origin of the system error. This way interpretation of the error should not leave questions + * open, even to foreign systems. */ + + error = abs(error); + const char *name = errno_to_name(error); + return sd_varlink_errorbo( v, SD_VARLINK_ERROR_SYSTEM, - SD_JSON_BUILD_PAIR("errno", SD_JSON_BUILD_INTEGER(abs(error)))); + SD_JSON_BUILD_PAIR_STRING("origin", "linux"), + SD_JSON_BUILD_PAIR_INTEGER("errno", error), + JSON_BUILD_PAIR_STRING_NON_EMPTY("errnoName", name)); } _public_ int sd_varlink_notify(sd_varlink *v, sd_json_variant *parameters) { @@ -4193,6 +4204,8 @@ _public_ int sd_varlink_error_to_errno(const char *error, sd_json_variant *param { SD_VARLINK_ERROR_EXPECTED_MORE, -EBADE }, }; + int r; + if (!error) return 0; @@ -4200,20 +4213,46 @@ _public_ int sd_varlink_error_to_errno(const char *error, sd_json_variant *param if (streq(error, t->error)) return t->value; - if (streq(error, SD_VARLINK_ERROR_SYSTEM) && parameters) { - sd_json_variant *e; + /* This following tries to reverse the operation sd_varlink_error_errno() applies to turn errnos into + * varlink errors */ + if (!streq(error, SD_VARLINK_ERROR_SYSTEM)) + return -EBADR; - e = sd_json_variant_by_key(parameters, "errno"); - if (sd_json_variant_is_integer(e)) { - int64_t i; + if (!parameters) + return -EBADR; - i = sd_json_variant_integer(e); - if (i > 0 && i < ERRNO_MAX) - return -i; - } + /* If an origin is set, check if it's Linux, otherwise don't translate */ + sd_json_variant *e = sd_json_variant_by_key(parameters, "origin"); + if (e && (!sd_json_variant_is_string(e) || + !streq(sd_json_variant_string(e), "linux"))) + return -EBADR; + + /* If a name is specified, go by name */ + e = sd_json_variant_by_key(parameters, "errnoName"); + if (e) { + if (!sd_json_variant_is_string(e)) + return -EBADR; + + r = errno_from_name(sd_json_variant_string(e)); + if (r < 0) + return -EBADR; + + assert(r > 0); + return -r; } - return -EBADR; /* Catch-all */ + /* Finally, use the provided error number, if there is one */ + e = sd_json_variant_by_key(parameters, "errno"); + if (!e) + return -EBADR; + if (!sd_json_variant_is_integer(e)) + return -EBADR; + + int64_t i = sd_json_variant_integer(e); + if (i <= 0 || i > ERRNO_MAX) + return -EBADR; + + return (int) -i; } _public_ int sd_varlink_error_is_invalid_parameter(const char *error, sd_json_variant *parameter, const char *name) { diff --git a/src/libsystemd/sd-varlink/varlink-io.systemd.c b/src/libsystemd/sd-varlink/varlink-io.systemd.c index e2511265a3..30c9495d40 100644 --- a/src/libsystemd/sd-varlink/varlink-io.systemd.c +++ b/src/libsystemd/sd-varlink/varlink-io.systemd.c @@ -10,12 +10,21 @@ static SD_VARLINK_DEFINE_ERROR(Protocol); /* This one we invented, and use for generically propagating system errors (errno) to clients */ static SD_VARLINK_DEFINE_ERROR( System, - SD_VARLINK_DEFINE_FIELD(errno, SD_VARLINK_INT, 0)); + SD_VARLINK_FIELD_COMMENT("The origin of this system error, typically 'linux' to indicate Linux error numbers."), + SD_VARLINK_DEFINE_FIELD(origin, SD_VARLINK_STRING, SD_VARLINK_NULLABLE), + SD_VARLINK_FIELD_COMMENT("The Linux error name, i.e. ENOENT, EHWPOISON or similar."), + SD_VARLINK_DEFINE_FIELD(errnoName, SD_VARLINK_STRING, SD_VARLINK_NULLABLE), + SD_VARLINK_FIELD_COMMENT("The numeric Linux error number. Typically the name is preferable, if specified."), + SD_VARLINK_DEFINE_FIELD(errno, SD_VARLINK_STRING, SD_VARLINK_NULLABLE)); SD_VARLINK_DEFINE_INTERFACE( io_systemd, "io.systemd", + SD_VARLINK_SYMBOL_COMMENT("Local error if a Varlink connection is disconnected (this never crosses the wire and is synthesized locally only)."), &vl_error_Disconnected, + SD_VARLINK_SYMBOL_COMMENT("A method call time-out has been reached (also synthesized locally, does not cross wire)"), &vl_error_TimedOut, + SD_VARLINK_SYMBOL_COMMENT("Some form of protocol error (also synthesized locally, does not cross wire)"), &vl_error_Protocol, + SD_VARLINK_SYMBOL_COMMENT("A generic Linux system error (\"errno\"s)."), &vl_error_System); From ed70124b2cdfa5e8e3a678f910361fbc0a5505d1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 7 Jan 2025 18:54:39 +0100 Subject: [PATCH 3/3] test-varlink: add superficial test that validates varlink/errno translation both ways --- src/test/test-varlink.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/test/test-varlink.c b/src/test/test-varlink.c index fe54ec08a5..0708ec1446 100644 --- a/src/test/test-varlink.c +++ b/src/test/test-varlink.c @@ -153,6 +153,16 @@ static int method_passfd(sd_varlink *link, sd_json_variant *parameters, sd_varli return sd_varlink_reply(link, ret); } +static int method_fail_with_errno(sd_varlink *link, sd_json_variant *parameters, sd_varlink_method_flags_t flags, void *userdata) { + int r; + + r = sd_varlink_dispatch(link, parameters, NULL, NULL); + if (r != 0) + return r; + + return sd_varlink_error_errno(link, EHWPOISON); +} + static int method_done(sd_varlink *link, sd_json_variant *parameters, sd_varlink_method_flags_t flags, void *userdata) { if (++n_done == 2) @@ -315,6 +325,8 @@ static void *thread(void *arg) { ASSERT_STREQ(sd_json_variant_string(sd_json_variant_by_key(o, "method")), "io.test.IDontExist"); ASSERT_STREQ(e, SD_VARLINK_ERROR_METHOD_NOT_FOUND); + ASSERT_OK(sd_varlink_call(c, "io.test.FailWithErrno", NULL, &o, &e)); + ASSERT_ERROR(sd_varlink_error_to_errno(e, o), EHWPOISON); flood_test(arg); assert_se(sd_varlink_send(c, "io.test.Done", NULL) >= 0); @@ -368,6 +380,7 @@ TEST(chat) { assert_se(sd_varlink_server_bind_method(s, "io.test.PassFD", method_passfd) >= 0); assert_se(sd_varlink_server_bind_method(s, "io.test.DoSomething", method_something) >= 0); assert_se(sd_varlink_server_bind_method(s, "io.test.DoSomethingMore", method_something_more) >= 0); + assert_se(sd_varlink_server_bind_method(s, "io.test.FailWithErrno", method_fail_with_errno) >= 0); assert_se(sd_varlink_server_bind_method(s, "io.test.Done", method_done) >= 0); assert_se(sd_varlink_server_bind_connect(s, on_connect) >= 0); assert_se(sd_varlink_server_listen_address(s, sp, 0600) >= 0);