From 75e42847813410e52c0206e2bcb4f203a3486f06 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Jan 2019 09:26:54 +0100 Subject: [PATCH] keyfile: rework handling of GObject properties from keyfile - previously, writer would use nm_keyfile_plugin_kf_set_integer() for G_TYPE_UINT types. That means, values larger than G_MAXINT would be stored as negative values. On the other hand, the reader would always reject negative values. Fix that, by parsing the integer ourself. Note that we still reject the old (negative) values and there is no compatibility for accepting such values. They were not accepted by reader in the past and so they are still rejected. This affects for example ethernet.mtu setting (arguably, the MTU is usually set to small values where the issue was not apparent). This is also covered by a test. - no longer use nm_keyfile_plugin_kf_set_integer(). nm_keyfile_plugin_kf_set_integer() calls g_key_file_get_integer(), which uses g_key_file_parse_integer_as_value(). That one has the odd behavior of accepting "" as valid. Note how that differs from g_key_file_parse_value_as_double() which rejects trailing data. Implement the parsing ourself. There are some changes here: - g_key_file_parse_value_as_integer() uses strtol() with base 10. We no longer require a certain the base, so '0x' hex values are allowed now as well. - bogus suffixes are now rejected but were accepted by g_key_file_parse_value_as_integer(). We however still accept leading and trailing whitespace, as before. - use nm_g_object_set_property*(). g_object_set() asserts that the value is in range. We cannot pass invalid values without checking that they are valid. - emit warnings when values cannot be parsed. Previously they would have been silently ignored or fail an assertion during g_object_set(). - don't use "helpers" like nm_keyfile_plugin_kf_set_uint64(). These merely call GKeyFile's setters (taking care of aliases). The setters of GKeyFile don't do anything miraculously, they merely call g_key_file_set_value() with the string that one would expect. Convert the numbers/boolean ourselfs. For one, we don't require a heap allocation to convert a number to string. Also, there is no point in leaving this GKeyFile API, because even if GKeyFile day would change, we still must continue to support the present format, as that is what users have on disk. So, even if a new way would be implemented by GKeyFile, the current way must forever be accepted too. Hence, we don't need this abstraction. --- libnm-core/nm-keyfile-utils.c | 2 - libnm-core/nm-keyfile-utils.h | 2 - libnm-core/nm-keyfile.c | 329 ++++++++++++++++++++------------ libnm-core/tests/test-setting.c | 13 +- 4 files changed, 217 insertions(+), 129 deletions(-) diff --git a/libnm-core/nm-keyfile-utils.c b/libnm-core/nm-keyfile-utils.c index bf841cc0e..9543ebde6 100644 --- a/libnm-core/nm-keyfile-utils.c +++ b/libnm-core/nm-keyfile-utils.c @@ -179,8 +179,6 @@ nm_keyfile_plugin_kf_set_##stype (GKeyFile *kf, \ } DEFINE_KF_WRAPPER(string, char*, const char*); -DEFINE_KF_WRAPPER(integer, int, int); -DEFINE_KF_WRAPPER(uint64, guint64, guint64); DEFINE_KF_WRAPPER(boolean, gboolean, gboolean); DEFINE_KF_WRAPPER(value, char*, const char*); diff --git a/libnm-core/nm-keyfile-utils.h b/libnm-core/nm-keyfile-utils.h index aa647cf79..0467230e4 100644 --- a/libnm-core/nm-keyfile-utils.h +++ b/libnm-core/nm-keyfile-utils.h @@ -67,8 +67,6 @@ void nm_keyfile_plugin_kf_set_##stype (GKeyFile *kf, \ const char *key, \ set_ctype value); DEFINE_KF_WRAPPER_PROTO(string, char*, const char*) -DEFINE_KF_WRAPPER_PROTO(integer, int, int) -DEFINE_KF_WRAPPER_PROTO(uint64, guint64, guint64) DEFINE_KF_WRAPPER_PROTO(boolean, gboolean, gboolean) DEFINE_KF_WRAPPER_PROTO(value, char*, const char*) diff --git a/libnm-core/nm-keyfile.c b/libnm-core/nm-keyfile.c index b376f842f..7617da44e 100644 --- a/libnm-core/nm-keyfile.c +++ b/libnm-core/nm-keyfile.c @@ -1384,55 +1384,119 @@ cert_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key) g_object_set (setting, key, bytes, NULL); } +static int +_parity_from_char (int ch) +{ +#if NM_MORE_ASSERTS > 5 + { + static char check = 0; + + if (check == 0) { + nm_auto_unref_gtypeclass GEnumClass *klass = g_type_class_ref (NM_TYPE_SETTING_SERIAL_PARITY); + guint i; + + check = 1; + + /* In older versions, parity was G_TYPE_CHAR/gint8, and the character + * value was stored as integer. + * For example parity=69 equals parity=E, meaning NM_SETTING_SERIAL_PARITY_EVEN. + * + * That means, certain values are reserved. Assert that these numbers + * are not reused when we extend NMSettingSerialParity enum. + * Actually, since NM_SETTING_SERIAL_PARITY is g_param_spec_enum(), + * we anyway cannot extend the enum without breaking API... + * + * [1] commit "a91e60902e libnm-core: make NMSettingSerial:parity an enum" + * [2] https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a91e60902eabae1de93d61323dae6ac894b5d40f + */ + g_assert (G_IS_ENUM_CLASS (klass)); + for (i = 0; i < klass->n_values; i++) { + const GEnumValue *v = &klass->values[i]; + int num = v->value; + + g_assert (_parity_from_char (num) == -1); + g_assert (!NM_IN_SET (num, 'e', 'E', 'o', 'O', 'n', 'N')); + } + } + } +#endif + + switch (ch) { + case 'E': + case 'e': + return NM_SETTING_SERIAL_PARITY_EVEN; + case 'O': + case 'o': + return NM_SETTING_SERIAL_PARITY_ODD; + case 'N': + case 'n': + return NM_SETTING_SERIAL_PARITY_NONE; + } + + return -1; +} + static void parity_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key) { const char *setting_name = nm_setting_get_name (setting); - NMSettingSerialParity parity; - int int_val; - gs_free char *str_val = NULL; + gs_free_error GError *err = NULL; + int parity; + gs_free char *tmp_str = NULL; + gint64 i64; /* Keyfile traditionally stored this as the ASCII value for 'E', 'o', or 'n'. * We now accept either that or the (case-insensitive) character itself (but * still always write it the old way, for backward compatibility). */ - int_val = nm_keyfile_plugin_kf_get_integer (info->keyfile, setting_name, key, NULL); - if (!int_val) { - str_val = nm_keyfile_plugin_kf_get_string (info->keyfile, setting_name, key, NULL); - if (str_val) { - if (str_val[0] && !str_val[1]) - int_val = str_val[0]; - else { - /* This will hit the warning below */ - int_val = 'X'; - } + tmp_str = nm_keyfile_plugin_kf_get_value (info->keyfile, setting_name, key, &err); + if (err) + goto out_err; + + if ( tmp_str + && tmp_str[0] != '\0' + && tmp_str[1] == '\0') { + /* the ASCII characters like 'E' are taken directly... */ + parity = _parity_from_char (tmp_str[0]); + if (parity >= 0) + goto parity_good; + } + + i64 = _nm_utils_ascii_str_to_int64 (tmp_str, 0, G_MININT, G_MAXINT, G_MININT64); + if ( i64 != G_MININT64 + && errno == 0) { + + if ((parity = _parity_from_char (i64)) >= 0) { + /* another oddity: the string is a valid number. However, if the numeric values + * is one of the supported ASCII codes, accept it (like 69 for 'E'). + */ + goto parity_good; } + + /* Finally, take the numeric value as is. */ + parity = i64; + goto parity_good; } - if (!int_val) - return; + handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid parity value '%s'"), + tmp_str ?: ""); + return; - switch (int_val) { - case 'E': - case 'e': - parity = NM_SETTING_SERIAL_PARITY_EVEN; - break; - case 'O': - case 'o': - parity = NM_SETTING_SERIAL_PARITY_ODD; - break; - case 'N': - case 'n': - parity = NM_SETTING_SERIAL_PARITY_NONE; - break; - default: - handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid parity value '%s'"), - str_val ?: ""); +parity_good: + nm_g_object_set_property_enum (G_OBJECT (setting), key, NM_TYPE_SETTING_SERIAL_PARITY, parity, &err); + +out_err: + if (!err) + return; + if ( err->domain == G_KEY_FILE_ERROR + && NM_IN_SET (err->code, G_KEY_FILE_ERROR_GROUP_NOT_FOUND, + G_KEY_FILE_ERROR_KEY_NOT_FOUND)) { + /* ignore such errors. The key is not present. */ return; } - - g_object_set (setting, key, parity, NULL); + handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid setting: %s"), err->message); } static void @@ -2531,11 +2595,13 @@ read_one_setting_value (NMSetting *setting, { KeyfileReaderInfo *info = user_data; GKeyFile *keyfile = info->keyfile; - const char *setting_name; - int errsv; - GType type; gs_free_error GError *err = NULL; const ParseInfoProperty *pip; + gs_free char *tmp_str = NULL; + const char *setting_name; + GType type; + guint64 u64; + gint64 i64; if (info->error) return; @@ -2581,62 +2647,71 @@ read_one_setting_value (NMSetting *setting, if (type == G_TYPE_STRING) { gs_free char *str_val = NULL; - str_val = nm_keyfile_plugin_kf_get_string (keyfile, setting_name, key, NULL); - g_object_set (setting, key, str_val, NULL); + str_val = nm_keyfile_plugin_kf_get_string (keyfile, setting_name, key, &err); + if (!err) + nm_g_object_set_property_string_take (G_OBJECT (setting), key, g_steal_pointer (&str_val), &err); } else if (type == G_TYPE_UINT) { - int int_val; - - int_val = nm_keyfile_plugin_kf_get_integer (keyfile, setting_name, key, NULL); - if (int_val < 0) { - if (!handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid negative value (%i)"), - int_val)) - return; + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); + if (!err) { + u64 = _nm_utils_ascii_str_to_uint64 (tmp_str, 0, 0, G_MAXUINT, G_MAXUINT64); + if ( u64 == G_MAXUINT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_uint (G_OBJECT (setting), key, u64, &err); } - g_object_set (setting, key, int_val, NULL); } else if (type == G_TYPE_INT) { - int int_val; - - int_val = nm_keyfile_plugin_kf_get_integer (keyfile, setting_name, key, NULL); - g_object_set (setting, key, int_val, NULL); + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); + if (!err) { + i64 = _nm_utils_ascii_str_to_int64 (tmp_str, 0, G_MININT, G_MAXINT, G_MININT64); + if ( i64 == G_MININT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_int (G_OBJECT (setting), key, i64, &err); + } } else if (type == G_TYPE_BOOLEAN) { gboolean bool_val; - bool_val = nm_keyfile_plugin_kf_get_boolean (keyfile, setting_name, key, NULL); - g_object_set (setting, key, bool_val, NULL); + bool_val = nm_keyfile_plugin_kf_get_boolean (keyfile, setting_name, key, &err); + if (!err) + nm_g_object_set_property_boolean (G_OBJECT (setting), key, bool_val, &err); } else if (type == G_TYPE_CHAR) { - int int_val; - - int_val = nm_keyfile_plugin_kf_get_integer (keyfile, setting_name, key, NULL); - if (int_val < G_MININT8 || int_val > G_MAXINT8) { - if (!handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid char value (%i)"), - int_val)) - return; + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); + if (!err) { + /* As documented by glib, G_TYPE_CHAR is really a (signed!) gint8. */ + i64 = _nm_utils_ascii_str_to_int64 (tmp_str, 0, G_MININT8, G_MAXINT8, G_MININT64); + if ( i64 == G_MININT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_char (G_OBJECT (setting), key, i64, &err); } - - g_object_set (setting, key, int_val, NULL); } else if (type == G_TYPE_UINT64) { - gs_free char *tmp_str = NULL; - guint64 uint_val; - - tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, NULL); - uint_val = g_ascii_strtoull (tmp_str, NULL, 10); - g_object_set (setting, key, uint_val, NULL); + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); + if (!err) { + u64 = _nm_utils_ascii_str_to_uint64 (tmp_str, 0, 0, G_MAXUINT64, G_MAXUINT64); + if ( u64 == G_MAXUINT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_uint64 (G_OBJECT (setting), key, u64, &err); + } } else if (type == G_TYPE_INT64) { - gs_free char *tmp_str = NULL; - gint64 int_val; - - tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, NULL); - int_val = _nm_utils_ascii_str_to_int64 (tmp_str, 10, G_MININT64, G_MAXINT64, 0); - errsv = errno; - if (errsv) { - if (!handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid int64 value (%s)"), - tmp_str)) - return; - } else - g_object_set (setting, key, int_val, NULL); + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); + if (!err) { + i64 = _nm_utils_ascii_str_to_int64 (tmp_str, 0, G_MININT64, G_MAXINT64, G_MAXINT64); + if ( i64 == G_MAXINT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_int64 (G_OBJECT (setting), key, i64, &err); + } } else if (type == G_TYPE_BYTES) { gs_free int *tmp = NULL; GByteArray *array; @@ -2679,31 +2754,39 @@ read_one_setting_value (NMSetting *setting, } else if (type == G_TYPE_ARRAY) { read_array_of_uint (keyfile, setting, key); } else if (G_VALUE_HOLDS_FLAGS (value)) { - guint64 uint_val; - - /* Flags are guint but GKeyFile has no uint reader, just uint64 */ - uint_val = nm_keyfile_plugin_kf_get_uint64 (keyfile, setting_name, key, &err); + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); if (!err) { - if (uint_val <= G_MAXUINT) - g_object_set (setting, key, (guint) uint_val, NULL); - else { - if (!handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("too large FLAGS property '%s' (%llu)"), - G_VALUE_TYPE_NAME (value), (unsigned long long) uint_val)) - return; - } + u64 = _nm_utils_ascii_str_to_uint64 (tmp_str, 0, 0, G_MAXUINT, G_MAXUINT64); + if ( u64 == G_MAXUINT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_flags (G_OBJECT (setting), key, type, u64, &err); } } else if (G_VALUE_HOLDS_ENUM (value)) { - int int_val; + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); + if (!err) { + i64 = _nm_utils_ascii_str_to_int64 (tmp_str, 0, G_MININT, G_MAXINT, G_MAXINT64); + if ( i64 == G_MAXINT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_enum (G_OBJECT (setting), key, type, i64, &err); + } + } else + g_return_if_reached (); - int_val = nm_keyfile_plugin_kf_get_integer (keyfile, setting_name, key, &err); - if (!err) - g_object_set (setting, key, (int) int_val, NULL); - } else { - if (!handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("unhandled setting property type '%s'"), - G_VALUE_TYPE_NAME (value))) - return; + if (err) { + if ( err->domain == G_KEY_FILE_ERROR + && NM_IN_SET (err->code, G_KEY_FILE_ERROR_GROUP_NOT_FOUND, + G_KEY_FILE_ERROR_KEY_NOT_FOUND)) { + /* ignore such errors. The key is not present. */ + } else { + handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid setting: %s"), err->message); + } } } @@ -2990,6 +3073,7 @@ write_setting_value (NMSetting *setting, GType type; const ParseInfoProperty *pip; GParamSpec *pspec; + char numstr[64]; if (info->error) return; @@ -3051,24 +3135,26 @@ write_setting_value (NMSetting *setting, str = g_value_get_string (value); if (str) nm_keyfile_plugin_kf_set_string (info->keyfile, setting_name, key, str); - } else if (type == G_TYPE_UINT) - nm_keyfile_plugin_kf_set_integer (info->keyfile, setting_name, key, (int) g_value_get_uint (value)); - else if (type == G_TYPE_INT) - nm_keyfile_plugin_kf_set_integer (info->keyfile, setting_name, key, g_value_get_int (value)); - else if (type == G_TYPE_UINT64) { - char numstr[30]; - + } else if (type == G_TYPE_UINT) { + nm_sprintf_buf (numstr, "%u", g_value_get_uint (value)); + nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); + } else if (type == G_TYPE_INT) { + nm_sprintf_buf (numstr, "%d", g_value_get_int (value)); + nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); + } else if (type == G_TYPE_UINT64) { nm_sprintf_buf (numstr, "%" G_GUINT64_FORMAT, g_value_get_uint64 (value)); nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); } else if (type == G_TYPE_INT64) { - char numstr[30]; - nm_sprintf_buf (numstr, "%" G_GINT64_FORMAT, g_value_get_int64 (value)); nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); } else if (type == G_TYPE_BOOLEAN) { - nm_keyfile_plugin_kf_set_boolean (info->keyfile, setting_name, key, g_value_get_boolean (value)); + nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, + g_value_get_boolean (value) + ? "true" + : "false"); } else if (type == G_TYPE_CHAR) { - nm_keyfile_plugin_kf_set_integer (info->keyfile, setting_name, key, (int) g_value_get_schar (value)); + nm_sprintf_buf (numstr, "%d", (int) g_value_get_schar (value)); + nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); } else if (type == G_TYPE_BYTES) { GBytes *bytes; const guint8 *data; @@ -3089,12 +3175,13 @@ write_setting_value (NMSetting *setting, } else if (type == G_TYPE_ARRAY) { write_array_of_uint (info->keyfile, setting, key, value); } else if (G_VALUE_HOLDS_FLAGS (value)) { - /* Flags are guint but GKeyFile has no uint reader, just uint64 */ - nm_keyfile_plugin_kf_set_uint64 (info->keyfile, setting_name, key, (guint64) g_value_get_flags (value)); - } else if (G_VALUE_HOLDS_ENUM (value)) - nm_keyfile_plugin_kf_set_integer (info->keyfile, setting_name, key, (int) g_value_get_enum (value)); - else - g_warn_if_reached (); + nm_sprintf_buf (numstr, "%u", g_value_get_flags (value)); + nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); + } else if (G_VALUE_HOLDS_ENUM (value)) { + nm_sprintf_buf (numstr, "%d", g_value_get_enum (value)); + nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); + } else + g_return_if_reached (); } GKeyFile * diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c index f9b4fc33d..66842db92 100644 --- a/libnm-core/tests/test-setting.c +++ b/libnm-core/tests/test-setting.c @@ -2078,7 +2078,7 @@ test_roundtrip_conversion (gconstpointer test_data) UUID, INTERFACE_NAME, (ETH_MTU != 0) - ? nm_sprintf_bufa (100, "mtu=%d\n", (int) ETH_MTU) + ? nm_sprintf_bufa (100, "mtu=%u\n", ETH_MTU) : "")); g_ptr_array_add (kf_data_arr, @@ -2106,7 +2106,7 @@ test_roundtrip_conversion (gconstpointer test_data) UUID, INTERFACE_NAME, (ETH_MTU != 0) - ? nm_sprintf_bufa (100, "mtu=%u\n", ETH_MTU) + ? nm_sprintf_bufa (100, "mtu=%d\n", (int) ETH_MTU) : "")); break; @@ -2143,8 +2143,13 @@ test_roundtrip_conversion (gconstpointer test_data) if ( ETH_MTU > (guint32) G_MAXINT && kf_data_idx == 1) { - /* values > 2^21 get written as signed integers. When reading this back, - * positive values are ignored. Patch the MTU in s_eth2. */ + /* older versions wrote values > 2^21 as signed integers, but the reader would + * always reject such negative values for G_TYPE_UINT. + * + * The test case kf_data_idx #1 still writes the values in the old style. + * The behavior was fixed, but such values are still rejected as invalid. + * + * Patch the setting so that the comparison below succeeds are usual. */ g_assert_cmpint (nm_setting_wired_get_mtu (s_eth2), ==, 0); g_object_set (s_eth2, NM_SETTING_WIRED_MTU,