From 681926ad433f41c546f88dcb9becc0d0b06cc20b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 25 Mar 2022 11:37:51 +0100 Subject: [PATCH 1/8] glib-aux: make nm_gobject_notify_together_full() macro more robust If __VA_ARGS__ contains odd arguments, it's not clear that N_ARG() gives the same as the array initialization. Add a static assert that the numbers agree to catch wrong usage of the macro. For example: nm_gobject_notify_together(setting, a, b, ); --- src/libnm-glib-aux/nm-macros-internal.h | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/libnm-glib-aux/nm-macros-internal.h b/src/libnm-glib-aux/nm-macros-internal.h index 140104d350..7cc8ac9797 100644 --- a/src/libnm-glib-aux/nm-macros-internal.h +++ b/src/libnm-glib-aux/nm-macros-internal.h @@ -620,10 +620,16 @@ nm_str_realloc(char *str) /* invokes _notify() for all arguments (of type _PropertyEnums). Note, that if * there are more than one prop arguments, this will involve a freeze/thaw * of GObject property notifications. */ -#define nm_gobject_notify_together_full(suffix, obj, ...) \ - _nm_gobject_notify_together_impl##suffix(obj, \ - NM_NARG(__VA_ARGS__), \ - (const _PropertyEnums##suffix[]){__VA_ARGS__}) +#define nm_gobject_notify_together_full(suffix, obj, ...) \ + G_STMT_START \ + { \ + const _PropertyEnums##suffix _props[] = {__VA_ARGS__}; \ + \ + G_STATIC_ASSERT(G_N_ELEMENTS(_props) == NM_NARG(__VA_ARGS__)); \ + \ + _nm_gobject_notify_together_impl##suffix(obj, G_N_ELEMENTS(_props), _props); \ + } \ + G_STMT_END #define nm_gobject_notify_together(obj, ...) nm_gobject_notify_together_full(, obj, __VA_ARGS__) From 22dcfb3a6770e9893440f6a99bed3aaf16e083b8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2022 14:59:00 +0100 Subject: [PATCH 2/8] libnm: fix update of cached option names in nm_setting_option_set() This is severe. We cache the list of names, and we must invalidate the cache when the names change. Otherwise, out-of-bound access and crash. Fixes: d0192b698e68 ('libnm: add nm_setting_option_set(), nm_setting_option_get_boolean(), nm_setting_option_set_boolean()') Fixes: 150af44e1042 ('libnm: add nm_setting_option_get_uint32(), nm_setting_option_set_uint32()') --- src/libnm-core-impl/nm-setting.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libnm-core-impl/nm-setting.c b/src/libnm-core-impl/nm-setting.c index e65e75e736..35070baed1 100644 --- a/src/libnm-core-impl/nm-setting.c +++ b/src/libnm-core-impl/nm-setting.c @@ -3975,7 +3975,7 @@ nm_setting_option_set(NMSetting *setting, const char *opt_name, GVariant *varian g_hash_table_insert(hash, g_strdup(opt_name), g_variant_ref_sink(variant)); if (changed_value) - _nm_setting_option_notify(setting, !changed_name); + _nm_setting_option_notify(setting, changed_name); } /** @@ -4012,7 +4012,7 @@ nm_setting_option_set_boolean(NMSetting *setting, const char *opt_name, gboolean g_hash_table_insert(hash, g_strdup(opt_name), g_variant_ref_sink(g_variant_new_boolean(value))); if (changed_value) - _nm_setting_option_notify(setting, !changed_name); + _nm_setting_option_notify(setting, changed_name); } /** @@ -4047,7 +4047,7 @@ nm_setting_option_set_uint32(NMSetting *setting, const char *opt_name, guint32 v g_hash_table_insert(hash, g_strdup(opt_name), g_variant_ref_sink(g_variant_new_uint32(value))); if (changed_value) - _nm_setting_option_notify(setting, !changed_name); + _nm_setting_option_notify(setting, changed_name); } /*****************************************************************************/ From e965aa2536eb216b35ce5e8457c0b6c597b21352 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2022 12:49:32 +0100 Subject: [PATCH 3/8] libnm: add nm_setting_8021x_scheme_vtable_by_setting_key() helper Add function to lookup the vtable by name. Implement a binary search. --- .../nm-meta-setting-base-impl.c | 62 +++++++++++++++++++ .../nm-meta-setting-base-impl.h | 4 +- .../nm-meta-setting-base-impl.c | 62 +++++++++++++++++++ .../nm-meta-setting-base-impl.h | 4 +- 4 files changed, 130 insertions(+), 2 deletions(-) diff --git a/src/libnm-core-impl/nm-meta-setting-base-impl.c b/src/libnm-core-impl/nm-meta-setting-base-impl.c index cd55779f11..69daa76c9d 100644 --- a/src/libnm-core-impl/nm-meta-setting-base-impl.c +++ b/src/libnm-core-impl/nm-meta-setting-base-impl.c @@ -153,6 +153,68 @@ const NMSetting8021xSchemeVtable nm_setting_8021x_scheme_vtable[] = { #undef _D }; +const NMSetting8021xSchemeVtable * +nm_setting_8021x_scheme_vtable_by_setting_key(const char *key) +{ + static const NMSetting8021xSchemeType sorted_index[] = { + NM_SETTING_802_1X_SCHEME_TYPE_CA_CERT, + NM_SETTING_802_1X_SCHEME_TYPE_CLIENT_CERT, + NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_CA_CERT, + NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_CLIENT_CERT, + NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_PRIVATE_KEY, + NM_SETTING_802_1X_SCHEME_TYPE_PRIVATE_KEY, + }; + int imin, imax; + + nm_assert(key); + + if (NM_MORE_ASSERT_ONCE(5)) { + const NMSetting8021xSchemeVtable *vtable_prev = NULL; + int i, j; + + for (i = 0; i < (int) G_N_ELEMENTS(sorted_index); i++) { + const NMSetting8021xSchemeType t = sorted_index[i]; + const NMSetting8021xSchemeVtable *vtable; + + nm_assert(_NM_INT_NOT_NEGATIVE(t)); + nm_assert(t < G_N_ELEMENTS(nm_setting_8021x_scheme_vtable) - 1); + + for (j = 0; j < i; j++) + nm_assert(t != sorted_index[j]); + + vtable = &nm_setting_8021x_scheme_vtable[t]; + + nm_assert(vtable->scheme_type == t); + nm_assert(vtable->setting_key); + + if (vtable_prev) + nm_assert(strcmp(vtable_prev->setting_key, vtable->setting_key) < 0); + vtable_prev = vtable; + } + } + + imin = 0; + imax = G_N_ELEMENTS(sorted_index) - 1; + while (imin <= imax) { + const NMSetting8021xSchemeVtable *vtable; + const int imid = imin + (imax - imin) / 2; + int cmp; + + vtable = &nm_setting_8021x_scheme_vtable[sorted_index[imid]]; + + cmp = strcmp(vtable->setting_key, key); + if (cmp == 0) + return vtable; + + if (cmp < 0) + imin = imid + 1; + else + imax = imid - 1; + } + + return NULL; +} + /*****************************************************************************/ const NMMetaSettingInfo nm_meta_setting_infos[] = { diff --git a/src/libnm-core-intern/nm-meta-setting-base-impl.h b/src/libnm-core-intern/nm-meta-setting-base-impl.h index b1f1263693..1268865954 100644 --- a/src/libnm-core-intern/nm-meta-setting-base-impl.h +++ b/src/libnm-core-intern/nm-meta-setting-base-impl.h @@ -51,7 +51,7 @@ typedef enum /*< skip >*/ { /*****************************************************************************/ -typedef enum { +typedef enum _nm_packed { NM_SETTING_802_1X_SCHEME_TYPE_CA_CERT, NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_CA_CERT, NM_SETTING_802_1X_SCHEME_TYPE_CLIENT_CERT, @@ -92,6 +92,8 @@ typedef struct { extern const NMSetting8021xSchemeVtable nm_setting_8021x_scheme_vtable[_NM_SETTING_802_1X_SCHEME_TYPE_NUM + 1]; +const NMSetting8021xSchemeVtable *nm_setting_8021x_scheme_vtable_by_setting_key(const char *key); + /*****************************************************************************/ typedef enum _nm_packed { diff --git a/src/libnmc-setting/nm-meta-setting-base-impl.c b/src/libnmc-setting/nm-meta-setting-base-impl.c index cd55779f11..69daa76c9d 100644 --- a/src/libnmc-setting/nm-meta-setting-base-impl.c +++ b/src/libnmc-setting/nm-meta-setting-base-impl.c @@ -153,6 +153,68 @@ const NMSetting8021xSchemeVtable nm_setting_8021x_scheme_vtable[] = { #undef _D }; +const NMSetting8021xSchemeVtable * +nm_setting_8021x_scheme_vtable_by_setting_key(const char *key) +{ + static const NMSetting8021xSchemeType sorted_index[] = { + NM_SETTING_802_1X_SCHEME_TYPE_CA_CERT, + NM_SETTING_802_1X_SCHEME_TYPE_CLIENT_CERT, + NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_CA_CERT, + NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_CLIENT_CERT, + NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_PRIVATE_KEY, + NM_SETTING_802_1X_SCHEME_TYPE_PRIVATE_KEY, + }; + int imin, imax; + + nm_assert(key); + + if (NM_MORE_ASSERT_ONCE(5)) { + const NMSetting8021xSchemeVtable *vtable_prev = NULL; + int i, j; + + for (i = 0; i < (int) G_N_ELEMENTS(sorted_index); i++) { + const NMSetting8021xSchemeType t = sorted_index[i]; + const NMSetting8021xSchemeVtable *vtable; + + nm_assert(_NM_INT_NOT_NEGATIVE(t)); + nm_assert(t < G_N_ELEMENTS(nm_setting_8021x_scheme_vtable) - 1); + + for (j = 0; j < i; j++) + nm_assert(t != sorted_index[j]); + + vtable = &nm_setting_8021x_scheme_vtable[t]; + + nm_assert(vtable->scheme_type == t); + nm_assert(vtable->setting_key); + + if (vtable_prev) + nm_assert(strcmp(vtable_prev->setting_key, vtable->setting_key) < 0); + vtable_prev = vtable; + } + } + + imin = 0; + imax = G_N_ELEMENTS(sorted_index) - 1; + while (imin <= imax) { + const NMSetting8021xSchemeVtable *vtable; + const int imid = imin + (imax - imin) / 2; + int cmp; + + vtable = &nm_setting_8021x_scheme_vtable[sorted_index[imid]]; + + cmp = strcmp(vtable->setting_key, key); + if (cmp == 0) + return vtable; + + if (cmp < 0) + imin = imid + 1; + else + imax = imid - 1; + } + + return NULL; +} + /*****************************************************************************/ const NMMetaSettingInfo nm_meta_setting_infos[] = { diff --git a/src/libnmc-setting/nm-meta-setting-base-impl.h b/src/libnmc-setting/nm-meta-setting-base-impl.h index b1f1263693..1268865954 100644 --- a/src/libnmc-setting/nm-meta-setting-base-impl.h +++ b/src/libnmc-setting/nm-meta-setting-base-impl.h @@ -51,7 +51,7 @@ typedef enum /*< skip >*/ { /*****************************************************************************/ -typedef enum { +typedef enum _nm_packed { NM_SETTING_802_1X_SCHEME_TYPE_CA_CERT, NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_CA_CERT, NM_SETTING_802_1X_SCHEME_TYPE_CLIENT_CERT, @@ -92,6 +92,8 @@ typedef struct { extern const NMSetting8021xSchemeVtable nm_setting_8021x_scheme_vtable[_NM_SETTING_802_1X_SCHEME_TYPE_NUM + 1]; +const NMSetting8021xSchemeVtable *nm_setting_8021x_scheme_vtable_by_setting_key(const char *key); + /*****************************************************************************/ typedef enum _nm_packed { From a0db72bf6d69459ad347c824e1848cf584733bef Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2022 12:50:27 +0100 Subject: [PATCH 4/8] keyfile: use nm_setting_8021x_scheme_vtable_by_setting_key() helper in cert_writer() --- src/libnm-core-impl/nm-keyfile.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/libnm-core-impl/nm-keyfile.c b/src/libnm-core-impl/nm-keyfile.c index dce53ae27e..a2ad57c72f 100644 --- a/src/libnm-core-impl/nm-keyfile.c +++ b/src/libnm-core-impl/nm-keyfile.c @@ -2689,16 +2689,10 @@ cert_writer_default(NMConnection *connection, static void cert_writer(KeyfileWriterInfo *info, NMSetting *setting, const char *key, const GValue *value) { - const NMSetting8021xSchemeVtable *vtable = NULL; + const NMSetting8021xSchemeVtable *vtable; const char *setting_name; - guint i; - for (i = 0; nm_setting_8021x_scheme_vtable[i].setting_key; i++) { - if (nm_streq0(nm_setting_8021x_scheme_vtable[i].setting_key, key)) { - vtable = &nm_setting_8021x_scheme_vtable[i]; - break; - } - } + vtable = nm_setting_8021x_scheme_vtable_by_setting_key(key); if (!vtable) g_return_if_reached(); From cfe594903ecfba7242043e8dd8a1c4c7c7f7f1fa Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2022 13:08:36 +0100 Subject: [PATCH 5/8] keyfile: simplify code path in write_setting_value() Avoid nested blocks. Check one condition after the other and handle it. --- src/libnm-core-impl/nm-keyfile.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/libnm-core-impl/nm-keyfile.c b/src/libnm-core-impl/nm-keyfile.c index a2ad57c72f..fb475cf23b 100644 --- a/src/libnm-core-impl/nm-keyfile.c +++ b/src/libnm-core-impl/nm-keyfile.c @@ -3849,6 +3849,14 @@ write_setting_value(KeyfileWriterInfo *info, _parse_info_find(setting, key, &setting_info, NULL, &pip); + if (pip && pip->has_writer_full) { + pip->writer_full(info, setting_info, property_info, pip, setting); + return; + } + + if (pip && pip->writer_skip) + return; + if (!pip) { if (!setting_info) { /* the setting type is unknown. That is highly unexpected @@ -3865,13 +3873,6 @@ write_setting_value(KeyfileWriterInfo *info, return; if (nm_streq(key, NM_SETTING_NAME)) return; - } else { - if (pip->has_writer_full) { - pip->writer_full(info, setting_info, property_info, pip, setting); - return; - } - if (pip->writer_skip) - return; } nm_assert(property_info->param_spec); From cec126979558e716dd3bfda91f5ff73db051e230 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2022 13:18:38 +0100 Subject: [PATCH 6/8] keyfile: rename handle_warn() to read_handle_warn() We will also want to warn during write. --- src/libnm-core-impl/nm-keyfile.c | 623 ++++++++++++++++--------------- 1 file changed, 318 insertions(+), 305 deletions(-) diff --git a/src/libnm-core-impl/nm-keyfile.c b/src/libnm-core-impl/nm-keyfile.c index fb475cf23b..e97d581ceb 100644 --- a/src/libnm-core-impl/nm-keyfile.c +++ b/src/libnm-core-impl/nm-keyfile.c @@ -115,12 +115,12 @@ _key_file_handler_data_init_write(NMKeyfileHandlerData *handler_data, &info->error); } -_nm_printf(5, 6) static void _handle_warn(KeyfileReaderInfo *info, - const char *kf_key, - const char *cur_property, - NMKeyfileWarnSeverity severity, - const char *fmt, - ...) +_nm_printf(5, 6) static void _read_handle_warn(KeyfileReaderInfo *info, + const char *kf_key, + const char *cur_property, + NMKeyfileWarnSeverity severity, + const char *fmt, + ...) { NMKeyfileHandlerData handler_data; @@ -148,16 +148,20 @@ _nm_printf(5, 6) static void _handle_warn(KeyfileReaderInfo *info, g_free(handler_data.warn.message); } -#define handle_warn(arg_info, arg_kf_key, arg_property_name, arg_severity, ...) \ - ({ \ - KeyfileReaderInfo *_info = (arg_info); \ - \ - nm_assert(!_info->error); \ - \ - if (_info->read_handler) { \ - _handle_warn(_info, (arg_kf_key), (arg_property_name), (arg_severity), __VA_ARGS__); \ - } \ - _info->error == NULL; \ +#define read_handle_warn(arg_info, arg_kf_key, arg_property_name, arg_severity, ...) \ + ({ \ + KeyfileReaderInfo *_info = (arg_info); \ + \ + nm_assert(!_info->error); \ + \ + if (_info->read_handler) { \ + _read_handle_warn(_info, \ + (arg_kf_key), \ + (arg_property_name), \ + (arg_severity), \ + __VA_ARGS__); \ + } \ + _info->error == NULL; \ }) /*****************************************************************************/ @@ -264,11 +268,11 @@ get_one_int(KeyfileReaderInfo *info, if (!str || !str[0]) { if (info) { - handle_warn(info, - kf_key, - property_name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("ignoring missing number")); + read_handle_warn(info, + kf_key, + property_name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("ignoring missing number")); } return FALSE; } @@ -276,12 +280,12 @@ get_one_int(KeyfileReaderInfo *info, tmp = _nm_utils_ascii_str_to_int64(str, 10, 0, max_val, -1); if (tmp == -1) { if (info) { - handle_warn(info, - kf_key, - property_name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("ignoring invalid number '%s'"), - str); + read_handle_warn(info, + kf_key, + property_name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("ignoring invalid number '%s'"), + str); } return FALSE; } @@ -305,13 +309,13 @@ build_address(KeyfileReaderInfo *info, addr = nm_ip_address_new(family, address_str, plen, &error); if (!addr) { - handle_warn(info, - kf_key, - property_name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("ignoring invalid %s address: %s"), - family == AF_INET ? "IPv4" : "IPv6", - error->message); + read_handle_warn(info, + kf_key, + property_name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("ignoring invalid %s address: %s"), + family == AF_INET ? "IPv4" : "IPv6", + error->message); g_error_free(error); } @@ -351,13 +355,13 @@ build_route(KeyfileReaderInfo *info, metric = u32; gateway_str = NULL; } else { - handle_warn(info, - kf_key, - property_name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("ignoring invalid gateway '%s' for %s route"), - gateway_str, - family == AF_INET ? "IPv4" : "IPv6"); + read_handle_warn(info, + kf_key, + property_name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("ignoring invalid gateway '%s' for %s route"), + gateway_str, + family == AF_INET ? "IPv4" : "IPv6"); return NULL; } } @@ -373,13 +377,13 @@ build_route(KeyfileReaderInfo *info, route = nm_ip_route_new(family, dest_str, plen, gateway_str, metric, &error); if (!route) { - handle_warn(info, - kf_key, - property_name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("ignoring invalid %s route: %s"), - family == AF_INET ? "IPv4" : "IPv6", - error->message); + read_handle_warn(info, + kf_key, + property_name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("ignoring invalid %s route: %s"), + family == AF_INET ? "IPv4" : "IPv6", + error->message); g_error_free(error); } @@ -551,15 +555,15 @@ read_one_ip_address_or_route(KeyfileReaderInfo *info, /* get address field */ address_str = read_field(¤t, &err_str, IP_ADDRESS_CHARS, DELIMITERS); if (err_str) { - handle_warn(info, - kf_key, - property_name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("unexpected character '%c' for address %s: '%s' (position %td)"), - *err_str, - kf_key, - VALUE_ORIG(), - err_str - current); + read_handle_warn(info, + kf_key, + property_name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("unexpected character '%c' for address %s: '%s' (position %td)"), + *err_str, + kf_key, + VALUE_ORIG(), + err_str - current); return NULL; } /* get prefix length field (skippable) */ @@ -567,30 +571,31 @@ read_one_ip_address_or_route(KeyfileReaderInfo *info, /* get gateway field */ gateway_str = read_field(¤t, &err_str, IP_ADDRESS_CHARS, DELIMITERS); if (err_str) { - handle_warn(info, - kf_key, - property_name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("unexpected character '%c' for %s: '%s' (position %td)"), - *err_str, - kf_key, - VALUE_ORIG(), - err_str - current); + read_handle_warn(info, + kf_key, + property_name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("unexpected character '%c' for %s: '%s' (position %td)"), + *err_str, + kf_key, + VALUE_ORIG(), + err_str - current); return NULL; } /* for routes, get metric */ if (route) { metric_str = read_field(¤t, &err_str, DIGITS, DELIMITERS); if (err_str) { - handle_warn(info, - kf_key, - property_name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("unexpected character '%c' in prefix length for %s: '%s' (position %td)"), - *err_str, - kf_key, - VALUE_ORIG(), - err_str - current); + read_handle_warn( + info, + kf_key, + property_name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("unexpected character '%c' in prefix length for %s: '%s' (position %td)"), + *err_str, + kf_key, + VALUE_ORIG(), + err_str - current); return NULL; } } else @@ -599,23 +604,23 @@ read_one_ip_address_or_route(KeyfileReaderInfo *info, /* there is still some data */ if (*current) { /* another field follows */ - handle_warn(info, - kf_key, - property_name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("garbage at the end of value %s: '%s'"), - kf_key, - VALUE_ORIG()); + read_handle_warn(info, + kf_key, + property_name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("garbage at the end of value %s: '%s'"), + kf_key, + VALUE_ORIG()); return NULL; } else { /* semicolon at the end of input */ - if (!handle_warn(info, - kf_key, - property_name, - NM_KEYFILE_WARN_SEVERITY_INFO, - _("deprecated semicolon at the end of value %s: '%s'"), - kf_key, - VALUE_ORIG())) + if (!read_handle_warn(info, + kf_key, + property_name, + NM_KEYFILE_WARN_SEVERITY_INFO, + _("deprecated semicolon at the end of value %s: '%s'"), + kf_key, + VALUE_ORIG())) return NULL; } } @@ -628,26 +633,26 @@ read_one_ip_address_or_route(KeyfileReaderInfo *info, if (!get_one_int(info, kf_key, property_name, plen_str, ipv6 ? 128 : 32, &plen)) { plen = DEFAULT_PREFIX(route, ipv6); if (info->error - || !handle_warn(info, - kf_key, - property_name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid prefix length for %s '%s', defaulting to %d"), - kf_key, - VALUE_ORIG(), - plen)) + || !read_handle_warn(info, + kf_key, + property_name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid prefix length for %s '%s', defaulting to %d"), + kf_key, + VALUE_ORIG(), + plen)) return NULL; } } else { plen = DEFAULT_PREFIX(route, ipv6); - if (!handle_warn(info, - kf_key, - property_name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("missing prefix length for %s '%s', defaulting to %d"), - kf_key, - VALUE_ORIG(), - plen)) + if (!read_handle_warn(info, + kf_key, + property_name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("missing prefix length for %s '%s', defaulting to %d"), + kf_key, + VALUE_ORIG(), + plen)) return NULL; } @@ -983,13 +988,13 @@ ip_routing_rule_parser_full(KeyfileReaderInfo *info, NULL, &local); if (!rule) { - if (!handle_warn(info, - build_list[i_build_list].s_key, - property_info->name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid value for \"%s\": %s"), - build_list[i_build_list].s_key, - local->message)) + if (!read_handle_warn(info, + build_list[i_build_list].s_key, + property_info->name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid value for \"%s\": %s"), + build_list[i_build_list].s_key, + local->message)) return; continue; } @@ -1061,13 +1066,13 @@ ip_dns_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) NMIPAddr addr; if (inet_pton(addr_family, list[i], &addr) <= 0) { - if (!handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("ignoring invalid DNS server IPv%c address '%s'"), - nm_utils_addr_family_to_char(addr_family), - list[i])) { + if (!read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("ignoring invalid DNS server IPv%c address '%s'"), + nm_utils_addr_family_to_char(addr_family), + list[i])) { do { nm_clear_g_free(&list[i]); } while (++i < length); @@ -1098,13 +1103,13 @@ ip6_addr_gen_mode_parser(KeyfileReaderInfo *info, NMSetting *setting, const char s, (int *) &addr_gen_mode, NULL)) { - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid option '%s', use one of [%s]"), - s, - "eui64,stable-privacy"); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid option '%s', use one of [%s]"), + s, + "eui64,stable-privacy"); return; } } else @@ -1160,7 +1165,11 @@ mac_address_parser(KeyfileReaderInfo *info, goto good_addr_bin; } - handle_warn(info, key, key, NM_KEYFILE_WARN_SEVERITY_WARN, _("ignoring invalid MAC address")); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("ignoring invalid MAC address")); return; good_addr_bin: @@ -1232,14 +1241,14 @@ read_hash_of_string(KeyfileReaderInfo *info, gs_free_error GError *error = NULL; if (!_nm_setting_bond_validate_option(name, value, &error)) { - if (!handle_warn(info, - kf_key, - name, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("ignoring invalid bond option %s%s%s = %s%s%s: %s"), - NM_PRINT_FMT_QUOTE_STRING(name), - NM_PRINT_FMT_QUOTE_STRING(value), - error->message)) + if (!read_handle_warn(info, + kf_key, + name, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("ignoring invalid bond option %s%s%s = %s%s%s: %s"), + NM_PRINT_FMT_QUOTE_STRING(name), + NM_PRINT_FMT_QUOTE_STRING(value), + error->message)) return; } else nm_setting_bond_add_option(NM_SETTING_BOND(setting), name, value); @@ -1426,7 +1435,7 @@ ssid_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) bytes = get_bytes(info, setting_name, key, FALSE, TRUE); if (!bytes) { - handle_warn(info, key, key, NM_KEYFILE_WARN_SEVERITY_WARN, _("ignoring invalid SSID")); + read_handle_warn(info, key, key, NM_KEYFILE_WARN_SEVERITY_WARN, _("ignoring invalid SSID")); return; } g_object_set(setting, key, bytes, NULL); @@ -1440,11 +1449,11 @@ password_raw_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key bytes = get_bytes(info, setting_name, key, FALSE, TRUE); if (!bytes) { - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("ignoring invalid raw password")); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("ignoring invalid raw password")); return; } g_object_set(setting, key, bytes, NULL); @@ -1585,7 +1594,11 @@ cert_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) bin = g_bytes_get_data(bytes, &bin_len); if (bin_len == 0) { if (!info->error) { - handle_warn(info, key, key, NM_KEYFILE_WARN_SEVERITY_WARN, _("invalid key/cert value")); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid key/cert value")); } return; } @@ -1596,12 +1609,12 @@ cert_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) if (nm_setting_802_1x_check_cert_scheme(bin, bin_len, NULL) != NM_SETTING_802_1X_CK_SCHEME_PATH) { - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid key/cert value path \"%s\""), - bin); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid key/cert value path \"%s\""), + bin); return; } @@ -1621,12 +1634,12 @@ cert_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) * then by invoking a callback (and possibly keyfile settings plugin would * collect the file names to be checked and check them later). */ if (!g_file_test(path2, G_FILE_TEST_EXISTS)) { - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_INFO_MISSING_FILE, - _("certificate or key file '%s' does not exist"), - path2); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_INFO_MISSING_FILE, + _("certificate or key file '%s' does not exist"), + path2); } return; } @@ -1634,12 +1647,12 @@ cert_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) if (HAS_SCHEME_PREFIX(bin, bin_len, NM_KEYFILE_CERT_SCHEME_PREFIX_PKCS11)) { if (nm_setting_802_1x_check_cert_scheme(bin, bin_len, NULL) != NM_SETTING_802_1X_CK_SCHEME_PKCS11) { - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid PKCS#11 URI \"%s\""), - bin); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid PKCS#11 URI \"%s\""), + bin); return; } @@ -1680,11 +1693,11 @@ cert_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) bin_decoded = g_base64_decode(cdata, &bin_decoded_len); if (bin_decoded_len == 0) { - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid key/cert value data:;base64, is not base64")); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid key/cert value data:;base64, is not base64")); return; } @@ -1693,11 +1706,11 @@ cert_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) /* The blob probably starts with "file://". Setting the cert data will confuse NMSetting8021x. * In fact this is a limitation of NMSetting8021x which does not support setting blobs that start * with file://. Just warn and return TRUE to signal that we ~handled~ the setting. */ - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid key/cert value data:;base64,file://")); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid key/cert value data:;base64,file://")); return; } @@ -1718,12 +1731,12 @@ cert_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) /* Warn if the certificate didn't exist */ if (!path_exists) { - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_INFO_MISSING_FILE, - _("certificate or key file '%s' does not exist"), - path); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_INFO_MISSING_FILE, + _("certificate or key file '%s' does not exist"), + path); } return; } @@ -1734,11 +1747,11 @@ cert_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) * Setting the cert data will confuse NMSetting8021x. * In fact, NMSetting8021x does not support setting such binary data, so just warn and * continue. */ - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid key/cert value is not a valid blob")); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid key/cert value is not a valid blob")); return; } @@ -1836,12 +1849,12 @@ parity_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) goto parity_good; } - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid parity value '%s'"), - tmp_str ?: ""); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid parity value '%s'"), + tmp_str ?: ""); return; parity_good: @@ -1858,12 +1871,12 @@ out_err: /* ignore such errors. The key is not present. */ return; } - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid setting: %s"), - err->message); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid setting: %s"), + err->message); } static void @@ -1878,12 +1891,12 @@ team_config_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) g_object_set(G_OBJECT(setting), key, conf, NULL); if (conf && !nm_setting_verify(setting, NULL, &error)) { - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("ignoring invalid team configuration: %s"), - error->message); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("ignoring invalid team configuration: %s"), + error->message); g_object_set(G_OBJECT(setting), key, NULL, NULL); } } @@ -1909,12 +1922,12 @@ bridge_vlan_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) for (iter = strv; *iter; iter++) { vlan = nm_bridge_vlan_from_str(*iter, &local); if (!vlan) { - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - "invalid bridge VLAN: %s", - local->message); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + "invalid bridge VLAN: %s", + local->message); g_clear_error(&local); continue; } @@ -1961,12 +1974,12 @@ qdisc_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) qdisc = nm_utils_tc_qdisc_from_str(qdisc_str, &err); if (!qdisc) { - handle_warn(info, - keys[i], - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid qdisc: %s"), - err->message); + read_handle_warn(info, + keys[i], + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid qdisc: %s"), + err->message); } else { g_ptr_array_add(qdiscs, qdisc); } @@ -2011,12 +2024,12 @@ tfilter_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) tfilter = nm_utils_tc_tfilter_from_str(tfilter_str, &err); if (!tfilter) { - handle_warn(info, - keys[i], - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid tfilter: %s"), - err->message); + read_handle_warn(info, + keys[i], + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid tfilter: %s"), + err->message); } else { g_ptr_array_add(tfilters, tfilter); } @@ -3175,12 +3188,12 @@ read_one_setting_value(KeyfileReaderInfo *info, && !nm_keyfile_plugin_kf_has_key(keyfile, setting_info->setting_name, key, &err)) { /* Key doesn't exist or an error occurred, thus nothing to do. */ if (err) { - if (!handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("error loading setting value: %s"), - err->message)) + if (!read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("error loading setting value: %s"), + err->message)) return; } return; @@ -3290,7 +3303,7 @@ read_one_setting_value(KeyfileReaderInfo *info, if (val > 255u) { if (!already_warned - && !handle_warn( + && !read_handle_warn( info, key, key, @@ -3350,12 +3363,12 @@ read_one_setting_value(KeyfileReaderInfo *info, if (nm_keyfile_error_is_not_found(err)) { /* ignore such errors. The key is not present. */ } else { - handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid setting: %s"), - err->message); + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid setting: %s"), + err->message); } } } @@ -3375,12 +3388,12 @@ _read_setting(KeyfileReaderInfo *info) type = nm_setting_lookup_type(alias); if (!type) { - handle_warn(info, - NULL, - NULL, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid setting name '%s'"), - info->group); + read_handle_warn(info, + NULL, + NULL, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid setting name '%s'"), + info->group); return; } @@ -3422,13 +3435,13 @@ _read_setting(KeyfileReaderInfo *info) variant_type = sett_info->detail.gendata_info->get_variant_type(sett_info, key, &local); if (!variant_type) { - if (!handle_warn(info, - key, - NULL, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid key '%s.%s'"), - info->group, - key)) + if (!read_handle_warn(info, + key, + NULL, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid key '%s.%s'"), + info->group, + key)) break; continue; } @@ -3438,13 +3451,13 @@ _read_setting(KeyfileReaderInfo *info) v = g_key_file_get_boolean(info->keyfile, info->group, key, &local); if (local) { - if (!handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("key '%s.%s' is not boolean"), - info->group, - key)) + if (!read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("key '%s.%s' is not boolean"), + info->group, + key)) break; continue; } @@ -3455,13 +3468,13 @@ _read_setting(KeyfileReaderInfo *info) v = g_key_file_get_uint64(info->keyfile, info->group, key, &local); if (local) { - if (!handle_warn(info, - key, - key, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("key '%s.%s' is not a uint32"), - info->group, - key)) + if (!read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("key '%s.%s' is not a uint32"), + info->group, + key)) break; continue; } @@ -3512,12 +3525,12 @@ _read_setting_wireguard_peer(KeyfileReaderInfo *info) || !nm_streq0(str, cstr)) { /* the group name must be identical to the normalized(!) key, so that it * is uniquely identified. */ - handle_warn(info, - NULL, - NM_SETTING_WIREGUARD_PEERS, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid peer public key in section '%s'"), - info->group); + read_handle_warn(info, + NULL, + NM_SETTING_WIREGUARD_PEERS, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid peer public key in section '%s'"), + info->group); return; } nm_wireguard_peer_set_public_key(peer, cstr, TRUE); @@ -3527,13 +3540,13 @@ _read_setting_wireguard_peer(KeyfileReaderInfo *info) str = nm_keyfile_plugin_kf_get_string(info->keyfile, info->group, key, NULL); if (str) { if (!nm_wireguard_peer_set_preshared_key(peer, str, FALSE)) { - if (!handle_warn(info, - key, - NM_SETTING_WIREGUARD_PEERS, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("key '%s.%s' is not a valid 256 bit key in base64 encoding"), - info->group, - key)) + if (!read_handle_warn(info, + key, + NM_SETTING_WIREGUARD_PEERS, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("key '%s.%s' is not a valid 256 bit key in base64 encoding"), + info->group, + key)) return; } nm_clear_g_free(&str); @@ -3550,13 +3563,13 @@ _read_setting_wireguard_peer(KeyfileReaderInfo *info) NULL); if (errno != ENODATA) { if (i64 == -1 || !_nm_setting_secret_flags_valid(i64)) { - if (!handle_warn(info, - key, - NM_SETTING_WIREGUARD_PEERS, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("key '%s.%s' is not a valid secret flag"), - info->group, - key)) + if (!read_handle_warn(info, + key, + NM_SETTING_WIREGUARD_PEERS, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("key '%s.%s' is not a valid secret flag"), + info->group, + key)) return; } else nm_wireguard_peer_set_preshared_key_flags(peer, i64); @@ -3573,13 +3586,13 @@ _read_setting_wireguard_peer(KeyfileReaderInfo *info) NULL); if (errno != ENODATA) { if (i64 == -1) { - if (!handle_warn(info, - key, - NM_SETTING_WIREGUARD_PEERS, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("key '%s.%s' is not a integer in range 0 to 2^32"), - info->group, - key)) + if (!read_handle_warn(info, + key, + NM_SETTING_WIREGUARD_PEERS, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("key '%s.%s' is not a integer in range 0 to 2^32"), + info->group, + key)) return; } else nm_wireguard_peer_set_persistent_keepalive(peer, i64); @@ -3589,13 +3602,13 @@ _read_setting_wireguard_peer(KeyfileReaderInfo *info) str = nm_keyfile_plugin_kf_get_string(info->keyfile, info->group, key, NULL); if (str && str[0]) { if (!nm_wireguard_peer_set_endpoint(peer, str, FALSE)) { - if (!handle_warn(info, - key, - NM_SETTING_WIREGUARD_PEERS, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("key '%s.%s' is not a valid endpoint"), - info->group, - key)) + if (!read_handle_warn(info, + key, + NM_SETTING_WIREGUARD_PEERS, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("key '%s.%s' is not a valid endpoint"), + info->group, + key)) return; } } @@ -3615,13 +3628,13 @@ _read_setting_wireguard_peer(KeyfileReaderInfo *info) nm_wireguard_peer_append_allowed_ip(peer, sa[i], TRUE); } if (has_error) { - if (!handle_warn(info, - key, - NM_SETTING_WIREGUARD_PEERS, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("key '%s.%s' has invalid allowed-ips"), - info->group, - key)) + if (!read_handle_warn(info, + key, + NM_SETTING_WIREGUARD_PEERS, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("key '%s.%s' has invalid allowed-ips"), + info->group, + key)) return; } } @@ -3630,13 +3643,13 @@ _read_setting_wireguard_peer(KeyfileReaderInfo *info) return; if (!nm_wireguard_peer_is_valid(peer, TRUE, TRUE, &error)) { - handle_warn(info, - NULL, - NM_SETTING_WIREGUARD_PEERS, - NM_KEYFILE_WARN_SEVERITY_WARN, - _("peer '%s' is invalid: %s"), - info->group, - error->message); + read_handle_warn(info, + NULL, + NM_SETTING_WIREGUARD_PEERS, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("peer '%s' is invalid: %s"), + info->group, + error->message); return; } From b07bf1a8bbf98687ade29a575437dd728cffcea7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2022 13:28:15 +0100 Subject: [PATCH 7/8] keyfile: add write_handle_warn() helper --- src/libnm-core-impl/nm-keyfile.c | 57 ++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/src/libnm-core-impl/nm-keyfile.c b/src/libnm-core-impl/nm-keyfile.c index e97d581ceb..08539beaad 100644 --- a/src/libnm-core-impl/nm-keyfile.c +++ b/src/libnm-core-impl/nm-keyfile.c @@ -115,6 +115,8 @@ _key_file_handler_data_init_write(NMKeyfileHandlerData *handler_data, &info->error); } +/*****************************************************************************/ + _nm_printf(5, 6) static void _read_handle_warn(KeyfileReaderInfo *info, const char *kf_key, const char *cur_property, @@ -166,6 +168,61 @@ _nm_printf(5, 6) static void _read_handle_warn(KeyfileReaderInfo *info, /*****************************************************************************/ +_nm_unused _nm_printf(6, 7) static void _write_handle_warn(KeyfileWriterInfo *info, + NMSetting *setting, + const char *kf_key, + const char *cur_property, + NMKeyfileWarnSeverity severity, + const char *fmt, + ...) +{ + NMKeyfileHandlerData handler_data; + + _key_file_handler_data_init_write(&handler_data, + NM_KEYFILE_HANDLER_TYPE_WARN, + info, + nm_setting_get_name(setting), + cur_property, + setting, + kf_key); + handler_data.warn = (NMKeyfileHandlerDataWarn){ + .severity = severity, + .message = NULL, + .fmt = fmt, + }; + + va_start(handler_data.warn.ap, fmt); + + info->write_handler(info->connection, + info->keyfile, + NM_KEYFILE_HANDLER_TYPE_WARN, + &handler_data, + info->user_data); + + va_end(handler_data.warn.ap); + + g_free(handler_data.warn.message); +} + +#define write_handle_warn(arg_info, arg_setting, arg_kf_key, arg_property_name, arg_severity, ...) \ + ({ \ + KeyfileWriterInfo *_info = (arg_info); \ + \ + nm_assert(!_info->error); \ + \ + if (_info->write_handler) { \ + _write_handle_warn(_info, \ + (arg_setting), \ + (arg_kf_key), \ + (arg_property_name), \ + (arg_severity), \ + __VA_ARGS__); \ + } \ + _info->error == NULL; \ + }) + +/*****************************************************************************/ + static gboolean _secret_flags_persist_secret(NMSettingSecretFlags flags) { From 782f2fa8ef86a99b2dd6a25be5e742792a5b7b16 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2022 13:10:50 +0100 Subject: [PATCH 8/8] keyfile: don't require verified profile in nm_keyfile_write() Previously, only the daemon was writing keyfiles, and it ensures that they are always valid. As we now have this function as public API of libnm, we should drop this restriction and write the profile the best we can. Granted, an invalid profile may not be expressed in keyfile format, and the result is undefined. But make the best of it. --- src/libnm-core-impl/nm-keyfile.c | 68 +++++--------- src/libnm-core-impl/tests/test-keyfile.c | 109 +++++++++++++++++++++-- 2 files changed, 126 insertions(+), 51 deletions(-) diff --git a/src/libnm-core-impl/nm-keyfile.c b/src/libnm-core-impl/nm-keyfile.c index 08539beaad..00fb8a335d 100644 --- a/src/libnm-core-impl/nm-keyfile.c +++ b/src/libnm-core-impl/nm-keyfile.c @@ -168,13 +168,13 @@ _nm_printf(5, 6) static void _read_handle_warn(KeyfileReaderInfo *info, /*****************************************************************************/ -_nm_unused _nm_printf(6, 7) static void _write_handle_warn(KeyfileWriterInfo *info, - NMSetting *setting, - const char *kf_key, - const char *cur_property, - NMKeyfileWarnSeverity severity, - const char *fmt, - ...) +_nm_printf(6, 7) static void _write_handle_warn(KeyfileWriterInfo *info, + NMSetting *setting, + const char *kf_key, + const char *cur_property, + NMKeyfileWarnSeverity severity, + const char *fmt, + ...) { NMKeyfileHandlerData handler_data; @@ -3929,14 +3929,8 @@ write_setting_value(KeyfileWriterInfo *info, if (!pip) { if (!setting_info) { - /* the setting type is unknown. That is highly unexpected - * (and as this is currently only called from NetworkManager - * daemon, not possible). - * - * Still, handle it gracefully, because later keyfile writer will become - * public API of libnm, where @setting is (untrusted) user input. - * - * Gracefully here just means: ignore the setting. */ + /* the setting type is unknown. Handle this gracefully by + * ignoring the setting. */ return; } if (!property_info->param_spec) @@ -4154,8 +4148,10 @@ _write_setting_wireguard(NMSetting *setting, KeyfileWriterInfo *info) * @user_data: argument for @handler. * @error: the #GError in case writing fails. * - * @connection must verify as a valid profile according to - * nm_connection_verify(). + * @connection should verify as a valid profile according to + * nm_connection_verify(). If it does not verify, the keyfile may + * be incomplete and the parser may not be able to fully recreate + * the original profile. * * Returns: (transfer full): a new #GKeyFile or %NULL on error. * @@ -4169,7 +4165,6 @@ nm_keyfile_write(NMConnection *connection, GError **error) { nm_auto_unref_keyfile GKeyFile *keyfile = NULL; - GError *local = NULL; KeyfileWriterInfo info; NMSetting **settings; int i; @@ -4179,28 +4174,6 @@ nm_keyfile_write(NMConnection *connection, g_return_val_if_fail(!error || !*error, NULL); g_return_val_if_fail(handler_flags == NM_KEYFILE_HANDLER_FLAGS_NONE, NULL); - /* Technically, we might not require that a profile is valid in - * order to serialize it. Like also nm_keyfile_read() does not - * ensure that the read profile validates. - * - * However, if the profile does not validate, then there might be - * unexpected edge cases when we try to serialize it. Edge cases - * that might result in dangerous crash. - * - * So, for now we require valid profiles. */ - if (!nm_connection_verify(connection, error ? &local : NULL)) { - if (error) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_FAILED, - _("the profile is not valid: %s"), - local->message); - g_error_free(local); - } else - nm_assert(!local); - return NULL; - } - keyfile = g_key_file_new(); info = (KeyfileWriterInfo){ @@ -4254,11 +4227,16 @@ nm_keyfile_write(NMConnection *connection, key, (guint64) g_variant_get_uint32(v)); } else { - /* BUG: The variant type is not implemented. Since the connection - * verifies, this can only mean we either wrongly didn't reject - * the connection as invalid, or we didn't properly implement the - * variant type. */ - nm_assert_not_reached(); + if (!write_handle_warn(&info, + setting, + NULL, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("unsupported option \"%s.%s\" of variant type %s"), + setting_name, + key, + g_variant_get_type_string(v))) + goto out_with_info_error; continue; } } diff --git a/src/libnm-core-impl/tests/test-keyfile.c b/src/libnm-core-impl/tests/test-keyfile.c index 9bd13ffc30..c163c4293b 100644 --- a/src/libnm-core-impl/tests/test-keyfile.c +++ b/src/libnm-core-impl/tests/test-keyfile.c @@ -5,16 +5,18 @@ #include "libnm-core-impl/nm-default-libnm-core.h" -#include "libnm-glib-aux/nm-json-aux.h" -#include "libnm-core-intern/nm-keyfile-utils.h" +#include "libnm-base/nm-ethtool-utils-base.h" #include "libnm-core-intern/nm-keyfile-internal.h" -#include "nm-simple-connection.h" -#include "nm-setting-connection.h" -#include "nm-setting-wired.h" +#include "libnm-core-intern/nm-keyfile-utils.h" +#include "libnm-glib-aux/nm-json-aux.h" #include "nm-setting-8021x.h" +#include "nm-setting-connection.h" +#include "nm-setting-ethtool.h" +#include "nm-setting-proxy.h" #include "nm-setting-team.h" #include "nm-setting-user.h" -#include "nm-setting-proxy.h" +#include "nm-setting-wired.h" +#include "nm-simple-connection.h" #include "libnm-glib-aux/nm-test-utils.h" @@ -887,6 +889,100 @@ test_bridge_port_vlans(void) /*****************************************************************************/ +typedef struct { + bool expect; + guint n_calls; +} InvalidOptionWriteData; + +static gboolean +_invalid_option_write_handler(NMConnection *connection, + GKeyFile *keyfile, + NMKeyfileHandlerType handler_type, + NMKeyfileHandlerData *handler_data, + void *user_data) +{ + InvalidOptionWriteData *data = user_data; + const char *message; + NMKeyfileWarnSeverity severity; + + g_assert(data); + g_assert(data->expect); + + g_assert(data->n_calls == 0); + data->n_calls++; + + switch (handler_type) { + case NM_KEYFILE_HANDLER_TYPE_WARN: + nm_keyfile_handler_data_warn_get(handler_data, &message, &severity); + g_assert(message && strstr(message, "ethtool.bogus")); + break; + default: + g_assert_not_reached(); + } + + return TRUE; +} + +static void +test_invalid_option(void) +{ + gs_unref_object NMConnection *con = NULL; + NMSetting *s_ethtool; + nm_auto_unref_keyfile GKeyFile *kf = NULL; + gs_free_error GError *error = NULL; + InvalidOptionWriteData data; + + con = nmtst_create_minimal_connection("test invalid option", + NULL, + NM_SETTING_WIRED_SETTING_NAME, + NULL); + + s_ethtool = nm_setting_ethtool_new(); + + nm_connection_add_setting(con, s_ethtool); + + nm_setting_option_set_boolean(s_ethtool, NM_ETHTOOL_OPTNAME_PAUSE_RX, TRUE); + + data = (InvalidOptionWriteData){}; + kf = nm_keyfile_write(con, + NM_KEYFILE_HANDLER_FLAGS_NONE, + _invalid_option_write_handler, + &data, + nmtst_get_rand_bool() ? &error : NULL); + nmtst_assert_success(kf, error); + nm_clear_pointer(&kf, g_key_file_unref); + + nmtst_connection_normalize(con); + + nmtst_assert_connection_verifies_without_normalization(con); + + data = (InvalidOptionWriteData){}; + kf = nm_keyfile_write(con, + NM_KEYFILE_HANDLER_FLAGS_NONE, + _invalid_option_write_handler, + &data, + nmtst_get_rand_bool() ? &error : NULL); + nmtst_assert_success(kf, error); + nm_clear_pointer(&kf, g_key_file_unref); + + nm_setting_option_set(s_ethtool, "bogus", g_variant_new_int64(0)); + + data = (InvalidOptionWriteData){ + .expect = TRUE, + }; + kf = nm_keyfile_write(con, + NM_KEYFILE_HANDLER_FLAGS_NONE, + _invalid_option_write_handler, + &data, + nmtst_get_rand_bool() ? &error : NULL); + nmtst_assert_success(kf, error); + nm_clear_pointer(&kf, g_key_file_unref); + + g_assert_cmpint(data.n_calls, ==, 1); +} + +/*****************************************************************************/ + NMTST_DEFINE(); int @@ -904,6 +1000,7 @@ main(int argc, char **argv) g_test_add_func("/core/keyfile/test_vpn/1", test_vpn_1); g_test_add_func("/core/keyfile/bridge/vlans", test_bridge_vlans); g_test_add_func("/core/keyfile/bridge-port/vlans", test_bridge_port_vlans); + g_test_add_func("/core/keyfile/invalid-option", test_invalid_option); return g_test_run(); }