From 8239edbb9b903dc802957763a8bab04480a491e1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 21 Dec 2016 20:19:13 +0100 Subject: [PATCH 1/8] keyfile: fix memleak in keyfile reader's read_array_of_uint() Fixes: 9559a7a26021efa7ef49681b93a3b3bd4eadcef6 --- libnm-core/nm-keyfile-reader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libnm-core/nm-keyfile-reader.c b/libnm-core/nm-keyfile-reader.c index f25fa0e1a..e68914d80 100644 --- a/libnm-core/nm-keyfile-reader.c +++ b/libnm-core/nm-keyfile-reader.c @@ -111,7 +111,7 @@ read_array_of_uint (GKeyFile *file, GArray *array = NULL; gsize length; int i; - gint *tmp; + gs_free int *tmp = NULL; tmp = nm_keyfile_plugin_kf_get_integer_list (file, nm_setting_get_name (setting), key, &length, NULL); array = g_array_sized_new (FALSE, FALSE, sizeof (guint32), length); From f4fb4d271f0ff3e972518215f733efef5d903cda Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 Jan 2017 14:41:30 +0100 Subject: [PATCH 2/8] keyfile/tests: add test for reading dcb connection Catches previously fixed memleak in read_array_of_uint() --- Makefile.am | 1 + .../tests/keyfiles/Test_dcb_connection | 33 +++++++++++++++++++ .../plugins/keyfile/tests/test-keyfile.c | 10 ++++++ 3 files changed, 44 insertions(+) create mode 100644 src/settings/plugins/keyfile/tests/keyfiles/Test_dcb_connection diff --git a/Makefile.am b/Makefile.am index 051791f13..1631c3c95 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1683,6 +1683,7 @@ EXTRA_DIST += \ src/settings/plugins/keyfile/tests/keyfiles/Test_Missing_ID_UUID \ src/settings/plugins/keyfile/tests/keyfiles/Test_Enum_Property \ src/settings/plugins/keyfile/tests/keyfiles/Test_Flags_Property \ + src/settings/plugins/keyfile/tests/keyfiles/Test_dcb_connection \ \ src/settings/plugins/keyfile/tests/keyfiles/test-ca-cert.pem \ src/settings/plugins/keyfile/tests/keyfiles/test-key-and-cert.pem diff --git a/src/settings/plugins/keyfile/tests/keyfiles/Test_dcb_connection b/src/settings/plugins/keyfile/tests/keyfiles/Test_dcb_connection new file mode 100644 index 000000000..16d4b45e1 --- /dev/null +++ b/src/settings/plugins/keyfile/tests/keyfiles/Test_dcb_connection @@ -0,0 +1,33 @@ +[connection] +id=dcb connection 1 +uuid=ac3c251e-260f-49b6-8ceb-12d37ea00751 +type=ethernet +autoconnect=false +permissions= +secondaries= + +[ethernet] +mac-address-blacklist= + +[dcb] +app-fcoe-flags=1 +app-fip-flags=1 +app-iscsi-flags=1 +priority-bandwidth=0;0;0;0;0;0;0;0; +priority-flow-control=0;0;0;0;0;0;0;0; +priority-flow-control-flags=1 +priority-group-bandwidth=100;0;0;0;0;0;0;0; +priority-group-flags=1 +priority-group-id=0;0;0;0;0;0;0;0; +priority-strict-bandwidth=0;0;0;0;0;0;0;0; +priority-traffic-class=0;0;0;0;0;0;0;0; + +[ipv4] +dns-search= +method=auto + +[ipv6] +addr-gen-mode=stable-privacy +dns-search= +ip6-privacy=0 +method=auto diff --git a/src/settings/plugins/keyfile/tests/test-keyfile.c b/src/settings/plugins/keyfile/tests/test-keyfile.c index 05979f774..f1102bd32 100644 --- a/src/settings/plugins/keyfile/tests/test-keyfile.c +++ b/src/settings/plugins/keyfile/tests/test-keyfile.c @@ -1786,6 +1786,14 @@ test_write_wired_8021x_tls_connection_blob (void) g_free (new_priv_key); } +static void +test_read_dcb_connection (void) +{ + gs_unref_object NMConnection *connection = NULL; + + connection = keyfile_read_connection_from_file (TEST_KEYFILES_DIR"/Test_dcb_connection"); +} + static void test_read_infiniband_connection (void) { @@ -2622,6 +2630,8 @@ int main (int argc, char **argv) g_test_add_func ("/keyfile/test_write_wired_8021x_tls_connection_path", test_write_wired_8021x_tls_connection_path); g_test_add_func ("/keyfile/test_write_wired_8021x_tls_connection_blob", test_write_wired_8021x_tls_connection_blob); + g_test_add_func ("/keyfile/test_read_dcb_connection", test_read_dcb_connection); + g_test_add_func ("/keyfile/test_read_infiniband_connection", test_read_infiniband_connection); g_test_add_func ("/keyfile/test_write_infiniband_connection", test_write_infiniband_connection); From f779c51f87748d80b75274468bc95791f5428d00 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 21 Dec 2016 17:42:49 +0100 Subject: [PATCH 3/8] shared: move nm_utils_strbuf_*() helper to shared/nm-utils --- shared/nm-utils/nm-shared-utils.c | 79 +++++++++++++++++++++++++++++++ shared/nm-utils/nm-shared-utils.h | 19 ++++++++ src/nm-core-utils.c | 77 ------------------------------ src/nm-core-utils.h | 17 ------- 4 files changed, 98 insertions(+), 94 deletions(-) diff --git a/shared/nm-utils/nm-shared-utils.c b/shared/nm-utils/nm-shared-utils.c index 84a5b18e2..38bb8181f 100644 --- a/shared/nm-utils/nm-shared-utils.c +++ b/shared/nm-utils/nm-shared-utils.c @@ -27,6 +27,85 @@ /*****************************************************************************/ +void +nm_utils_strbuf_append_c (char **buf, gsize *len, char c) +{ + switch (*len) { + case 0: + return; + case 1: + (*buf)[0] = '\0'; + *len = 0; + (*buf)++; + return; + default: + (*buf)[0] = c; + (*buf)[1] = '\0'; + (*len)--; + (*buf)++; + return; + } +} + +void +nm_utils_strbuf_append_str (char **buf, gsize *len, const char *str) +{ + gsize src_len; + + switch (*len) { + case 0: + return; + case 1: + if (!str || !*str) { + (*buf)[0] = '\0'; + return; + } + (*buf)[0] = '\0'; + *len = 0; + (*buf)++; + return; + default: + if (!str || !*str) { + (*buf)[0] = '\0'; + return; + } + src_len = g_strlcpy (*buf, str, *len); + if (src_len >= *len) { + *buf = &(*buf)[*len]; + *len = 0; + } else { + *buf = &(*buf)[src_len]; + *len -= src_len; + } + return; + } +} + +void +nm_utils_strbuf_append (char **buf, gsize *len, const char *format, ...) +{ + char *p = *buf; + va_list args; + gint retval; + + if (*len == 0) + return; + + va_start (args, format); + retval = g_vsnprintf (p, *len, format, args); + va_end (args); + + if (retval >= *len) { + *buf = &p[*len]; + *len = 0; + } else { + *buf = &p[retval]; + *len -= retval; + } +} + +/*****************************************************************************/ + /* _nm_utils_ascii_str_to_int64: * * A wrapper for g_ascii_strtoll, that checks whether the whole string diff --git a/shared/nm-utils/nm-shared-utils.h b/shared/nm-utils/nm-shared-utils.h index f77fb0e35..5d8a3a86a 100644 --- a/shared/nm-utils/nm-shared-utils.h +++ b/shared/nm-utils/nm-shared-utils.h @@ -24,6 +24,25 @@ /*****************************************************************************/ +static inline void +_nm_utils_strbuf_init (char *buf, gsize len, char **p_buf_ptr, gsize *p_buf_len) +{ + NM_SET_OUT (p_buf_len, len); + NM_SET_OUT (p_buf_ptr, buf); + buf[0] = '\0'; +} + +#define nm_utils_strbuf_init(buf, p_buf_ptr, p_buf_len) \ + G_STMT_START { \ + G_STATIC_ASSERT (G_N_ELEMENTS (buf) == sizeof (buf) && sizeof (buf) > sizeof (char *)); \ + _nm_utils_strbuf_init ((buf), sizeof (buf), (p_buf_ptr), (p_buf_len)); \ + } G_STMT_END +void nm_utils_strbuf_append (char **buf, gsize *len, const char *format, ...) _nm_printf (3, 4); +void nm_utils_strbuf_append_c (char **buf, gsize *len, char c); +void nm_utils_strbuf_append_str (char **buf, gsize *len, const char *str); + +/*****************************************************************************/ + gint64 _nm_utils_ascii_str_to_int64 (const char *str, guint base, gint64 min, gint64 max, gint64 fallback); gint _nm_utils_ascii_str_to_bool (const char *str, diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 70af93383..a1f01b3f8 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -1771,83 +1771,6 @@ nm_utils_to_string_buffer_init_null (gconstpointer obj, char **buf, gsize *len) return TRUE; } -void -nm_utils_strbuf_append_c (char **buf, gsize *len, char c) -{ - switch (*len) { - case 0: - return; - case 1: - (*buf)[0] = '\0'; - *len = 0; - (*buf)++; - return; - default: - (*buf)[0] = c; - (*buf)[1] = '\0'; - (*len)--; - (*buf)++; - return; - } -} - -void -nm_utils_strbuf_append_str (char **buf, gsize *len, const char *str) -{ - gsize src_len; - - switch (*len) { - case 0: - return; - case 1: - if (!str || !*str) { - (*buf)[0] = '\0'; - return; - } - (*buf)[0] = '\0'; - *len = 0; - (*buf)++; - return; - default: - if (!str || !*str) { - (*buf)[0] = '\0'; - return; - } - src_len = g_strlcpy (*buf, str, *len); - if (src_len >= *len) { - *buf = &(*buf)[*len]; - *len = 0; - } else { - *buf = &(*buf)[src_len]; - *len -= src_len; - } - return; - } -} - -void -nm_utils_strbuf_append (char **buf, gsize *len, const char *format, ...) -{ - char *p = *buf; - va_list args; - gint retval; - - if (*len == 0) - return; - - va_start (args, format); - retval = g_vsnprintf (p, *len, format, args); - va_end (args); - - if (retval >= *len) { - *buf = &p[*len]; - *len = 0; - } else { - *buf = &p[retval]; - *len -= retval; - } -} - const char * nm_utils_flags2str (const NMUtilsFlags2StrDesc *descs, gsize n_descs, diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index f9d30b5ce..66bcdc29e 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -262,23 +262,6 @@ fcn_name (lookup_type val) \ /*****************************************************************************/ -static inline void -_nm_utils_strbuf_init (char *buf, gsize len, char **p_buf_ptr, gsize *p_buf_len) -{ - NM_SET_OUT (p_buf_len, len); - NM_SET_OUT (p_buf_ptr, buf); - buf[0] = '\0'; -} - -#define nm_utils_strbuf_init(buf, p_buf_ptr, p_buf_len) \ - G_STMT_START { \ - G_STATIC_ASSERT (G_N_ELEMENTS (buf) == sizeof (buf) && sizeof (buf) > sizeof (char *)); \ - _nm_utils_strbuf_init ((buf), sizeof (buf), (p_buf_ptr), (p_buf_len)); \ - } G_STMT_END -void nm_utils_strbuf_append (char **buf, gsize *len, const char *format, ...) _nm_printf (3, 4); -void nm_utils_strbuf_append_c (char **buf, gsize *len, char c); -void nm_utils_strbuf_append_str (char **buf, gsize *len, const char *str); - const char *nm_utils_get_ip_config_method (NMConnection *connection, GType ip_setting_type); From 5e7b14af03fef98566367c211c0e7f75ad9a9525 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 21 Dec 2016 16:27:24 +0100 Subject: [PATCH 4/8] keyfile: refactor parsing in get_bytes() to replace regex No longer use a regex to pre-evaluate whether @tmp_string looks like a integer list. Instead, parse the integer list ourself. First, drop the nm_keyfile_plugin_kf_has_key() check. Note that this merely verifies that such a key exits. It's rather pointless, because get_bytes() is only called for existing keys. Still, in case the check would actually yield differing results from the following nm_keyfile_plugin_kf_get_string(), we want to act depending on what nm_keyfile_plugin_kf_get_string() returns. Note that nm_keyfile_plugin_kf_get_string() looks up the key, alternatively fallback to the settings alias. Then, GKeyFile would parse the raw keyfile value and return it as string. Previously, we would first decide whether @tmp_string look like a integer list to decide wether to parse it via nm_keyfile_plugin_kf_get_integer_list(). But note that it's not clear that nm_keyfile_plugin_kf_get_integer_list() operates on the same string as nm_keyfile_plugin_kf_get_string(). Could it decide to return different strings based on whether such a key exists? E.g. when setting "802-11-wireless.ssid=foo" and "wifi.ssid=60;" they clearly would yield differing results: "foo" vs. [60]. Ok, probably it is not an issue because we call first nm_keyfile_plugin_kf_get_string(), decide whether it looks like a integer list, and return "foo" right away. This is still confusing and relyies on knowledge about how the value is encoded as string-list. Likewise, could our regex determine that the value looks like a integer list but then the integer list is unable to parse it? Certainly that can happen for values larger then 255. Just make it consistent. Get *one* @tmp_string. Try (manually) to interpret it as string list, or bail using it as plain text. Also, allow returning empty GBytes arrays. If somebody specifies an empty list, it's empty. Not NULL. --- libnm-core/nm-keyfile-reader.c | 189 ++++++++++++++++++++------------- 1 file changed, 116 insertions(+), 73 deletions(-) diff --git a/libnm-core/nm-keyfile-reader.c b/libnm-core/nm-keyfile-reader.c index e68914d80..15d8c8895 100644 --- a/libnm-core/nm-keyfile-reader.c +++ b/libnm-core/nm-keyfile-reader.c @@ -706,19 +706,18 @@ read_hash_of_string (GKeyFile *file, NMSetting *setting, const char *key) g_strfreev (keys); } -static void +static gsize unescape_semicolons (char *str) { - int i; - gsize len = strlen (str); + gsize i, j; - for (i = 0; i < len; i++) { - if (str[i] == '\\' && str[i+1] == ';') { - memmove(str + i, str + i + 1, len - (i + 1)); - len--; - } - str[len] = '\0'; + for (i = 0, j = 0; str[i]; ) { + if (str[i] == '\\' && str[i+1] == ';') + i++; + str[j++] = str[i++];; } + str[j] = '\0'; + return j; } static GBytes * @@ -728,77 +727,121 @@ get_bytes (KeyfileReaderInfo *info, gboolean zero_terminate, gboolean unescape_semicolon) { - GByteArray *array = NULL; - char *tmp_string; - gint *tmp_list; + gs_free char *tmp_string = NULL; + gboolean may_be_int_list = TRUE; gsize length; - int i; - - if (!nm_keyfile_plugin_kf_has_key (info->keyfile, setting_name, key, NULL)) - return NULL; /* New format: just a string * Old format: integer list; e.g. 11;25;38; */ tmp_string = nm_keyfile_plugin_kf_get_string (info->keyfile, setting_name, key, NULL); - if (tmp_string) { - GRegex *regex; - GMatchInfo *match_info; - const char *pattern = "^[[:space:]]*[[:digit:]]{1,3}[[:space:]]*;([[:space:]]*[[:digit:]]{1,3}[[:space:]]*;)*([[:space:]]*)?$"; - - regex = g_regex_new (pattern, 0, 0, NULL); - g_regex_match (regex, tmp_string, 0, &match_info); - if (!g_match_info_matches (match_info)) { - /* Handle as a simple string (ie, new format) */ - if (unescape_semicolon) - unescape_semicolons (tmp_string); - length = strlen (tmp_string); - if (zero_terminate) - length++; - array = g_byte_array_sized_new (length); - g_byte_array_append (array, (guint8 *) tmp_string, length); - } - g_match_info_free (match_info); - g_regex_unref (regex); - g_free (tmp_string); - } - - if (!array) { - gboolean already_warned = FALSE; - - /* Old format; list of ints */ - tmp_list = nm_keyfile_plugin_kf_get_integer_list (info->keyfile, setting_name, key, &length, NULL); - if (!tmp_list) { - handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("ignoring invalid binary property")); - return NULL; - } - array = g_byte_array_sized_new (length); - for (i = 0; i < length; i++) { - int val = tmp_list[i]; - unsigned char v = (unsigned char) (val & 0xFF); - - if (val < 0 || val > 255) { - if ( !already_warned - && !handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("ignoring invalid byte element '%d' (not between 0 and 255 inclusive)"), - val)) { - g_free (tmp_list); - g_byte_array_free (array, TRUE); - return NULL; - } - already_warned = TRUE; - } else - g_byte_array_append (array, (const unsigned char *) &v, sizeof (v)); - } - g_free (tmp_list); - } - - if (array->len == 0) { - g_byte_array_free (array, TRUE); + if (!tmp_string) return NULL; - } else - return g_byte_array_free_to_bytes (array); + + /* if the string is empty, we return an empty GBytes array. + * Note that for NM_SETTING_802_1X_PASSWORD_RAW both %NULL and + * an empty GBytes are valid, and shall be destinguished. */ + if (!tmp_string[0]) { + /* note that even if @zero_terminate is TRUE, we return an empty + * byte-array. The reason is that zero_terminate is there to terminate + * *valid* strings. It's not there to terminated invalid (empty) strings. + */ + return g_bytes_new_take (tmp_string, 0); + } + + for (length = 0; tmp_string[length]; length++) { + const char ch = tmp_string[length]; + + if ( !g_ascii_isspace (ch) + && !g_ascii_isdigit (ch) + && ch != ';') { + may_be_int_list = FALSE; + length += strlen (&tmp_string[length]); + break; + } + } + + /* Try to parse the string as a integer list. */ + if (may_be_int_list && length > 0) { + gs_free guint8 *bin_data = NULL; + const char *const s = tmp_string; + gsize i, d; + const gsize BIN_DATA_LEN = (length / 2 + 3); + + bin_data = g_malloc (BIN_DATA_LEN); + +#define DIGIT(c) ((c) - '0') + i = 0; + d = 0; + while (TRUE) { + int n; + + /* leading whitespace */ + while (g_ascii_isspace (s[i])) + i++; + if (s[i] == '\0') + break; + /* then expect 1 to 3 digits */ + if (!g_ascii_isdigit (s[i])) { + d = 0; + break; + } + n = DIGIT (s[i]); + i++; + if (g_ascii_isdigit (s[i])) { + n = 10 * n + DIGIT (s[i]); + i++; + if (g_ascii_isdigit (s[i])) { + n = 10 * n + DIGIT (s[i]); + i++; + } + } + if (n > 255) { + d = 0; + break; + } + + bin_data[d++] = n; + nm_assert (d < BIN_DATA_LEN); + + /* allow whitespace after the digit. */ + while (g_ascii_isspace (s[i])) + i++; + /* need a semicolon as separator. */ + if (s[i] != ';') { + d = 0; + break; + } + i++; + } +#undef DIGIT + + /* Old format; list of ints. We already did a strict validation of the + * string format before. We expect that this conversion cannot fail. */ + if (d > 0) { + /* note that @zero_terminate does not add a terminating '\0' to + * binary data as an integer list. + * + * But we add a '\0' to the bin_data pointer, just to avoid somebody + * (erronously!) reading the binary data as C-string. + * + * @d itself does not entail the '\0'. */ + nm_assert (d + 1 <= BIN_DATA_LEN); + bin_data = g_realloc (bin_data, d + 1); + bin_data[d] = '\0'; + return g_bytes_new_take (g_steal_pointer (&bin_data), d); + } + } + + /* Handle as a simple string (ie, new format) */ + if (unescape_semicolon) + length = unescape_semicolons (tmp_string); + if (zero_terminate) + length++; + if (length == 0) + return NULL; + tmp_string = g_realloc (tmp_string, length + (zero_terminate ? 0 : 1)); + return g_bytes_new_take (g_steal_pointer (&tmp_string), length); } static void From e965718ddd7069a13da7b7224a1563dcd24a17f6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 21 Dec 2016 19:43:44 +0100 Subject: [PATCH 5/8] keyfile: add nm_keyfile_plugin_kf_set_integer_list_uint8() helper --- libnm-core/nm-keyfile-utils.c | 24 ++++++++++++++++++++++++ libnm-core/nm-keyfile-utils.h | 6 ++++++ 2 files changed, 30 insertions(+) diff --git a/libnm-core/nm-keyfile-utils.c b/libnm-core/nm-keyfile-utils.c index 79b6ea112..13cf61616 100644 --- a/libnm-core/nm-keyfile-utils.c +++ b/libnm-core/nm-keyfile-utils.c @@ -112,6 +112,30 @@ nm_keyfile_plugin_kf_set_##stype##_list (GKeyFile *kf, \ DEFINE_KF_LIST_WRAPPER(integer, gint*, gint); DEFINE_KF_LIST_WRAPPER(string, gchar **, const gchar* const); +void +nm_keyfile_plugin_kf_set_integer_list_uint8 (GKeyFile *kf, + const char *group, + const char *key, + const guint8 *data, + gsize length) +{ + gsize i; + gsize l = length * 4 + 2; + gs_free char *value = g_malloc (l); + char *s = value; + + g_return_if_fail (kf); + g_return_if_fail (!length || data); + g_return_if_fail (group && group[0]); + g_return_if_fail (key && key[0]); + + value[0] = '\0'; + for (i = 0; i < length; i++) + nm_utils_strbuf_append (&s, &l, "%d;", (int) data[i]); + nm_assert (l > 0); + nm_keyfile_plugin_kf_set_value (kf, group, key, value); +} + /* Single value helpers */ #define DEFINE_KF_WRAPPER(stype, get_ctype, set_ctype) \ get_ctype \ diff --git a/libnm-core/nm-keyfile-utils.h b/libnm-core/nm-keyfile-utils.h index e7d803ce1..89fd3041a 100644 --- a/libnm-core/nm-keyfile-utils.h +++ b/libnm-core/nm-keyfile-utils.h @@ -47,6 +47,12 @@ void nm_keyfile_plugin_kf_set_##stype##_list (GKeyFile *kf, \ DEFINE_KF_LIST_WRAPPER_PROTO(integer, gint*, gint) DEFINE_KF_LIST_WRAPPER_PROTO(string, gchar**, const gchar* const) +void nm_keyfile_plugin_kf_set_integer_list_uint8 (GKeyFile *kf, + const char *group, + const char *key, + const guint8 *list, + gsize length); + /* Single-value helpers */ #define DEFINE_KF_WRAPPER_PROTO(stype, get_ctype, set_ctype) \ get_ctype nm_keyfile_plugin_kf_get_##stype (GKeyFile *kf, \ From 138d1e3b7b9e549bf07f4ced66fc1d80d3e1134f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 21 Dec 2016 19:48:19 +0100 Subject: [PATCH 6/8] keyfile: use nm_keyfile_plugin_kf_set_integer_list_uint8() helper --- libnm-core/nm-keyfile-writer.c | 39 +++++++++------------------------- 1 file changed, 10 insertions(+), 29 deletions(-) diff --git a/libnm-core/nm-keyfile-writer.c b/libnm-core/nm-keyfile-writer.c index 4a2fbd1f1..d0e3effb4 100644 --- a/libnm-core/nm-keyfile-writer.c +++ b/libnm-core/nm-keyfile-writer.c @@ -295,7 +295,7 @@ ssid_writer (KeyfileWriterInfo *info, const char *setting_name = nm_setting_get_name (setting); gboolean new_format = TRUE; unsigned int semicolons = 0; - int i, *tmp_array; + gsize i; char *ssid; g_return_if_fail (G_VALUE_HOLDS (value, G_TYPE_BYTES)); @@ -327,7 +327,8 @@ ssid_writer (KeyfileWriterInfo *info, else { /* Escape semicolons with backslashes to make strings * containing ';', such as '16;17;' unambiguous */ - int j = 0; + gsize j = 0; + for (i = 0; i < ssid_len; i++) { if (ssid_data[i] == ';') ssid[j++] = '\\'; @@ -336,13 +337,8 @@ ssid_writer (KeyfileWriterInfo *info, } nm_keyfile_plugin_kf_set_string (info->keyfile, setting_name, key, ssid); g_free (ssid); - } else { - tmp_array = g_new (gint, ssid_len); - for (i = 0; i < ssid_len; i++) - tmp_array[i] = (int) ssid_data[i]; - nm_keyfile_plugin_kf_set_integer_list (info->keyfile, setting_name, key, tmp_array, ssid_len); - g_free (tmp_array); - } + } else + nm_keyfile_plugin_kf_set_integer_list_uint8 (info->keyfile, setting_name, key, ssid_data, ssid_len); } static void @@ -353,9 +349,8 @@ password_raw_writer (KeyfileWriterInfo *info, { const char *setting_name = nm_setting_get_name (setting); GBytes *array; - int *tmp_array; - gsize i, len; - const char *data; + gsize len; + const guint8 *data; g_return_if_fail (G_VALUE_HOLDS (value, G_TYPE_BYTES)); @@ -365,12 +360,7 @@ password_raw_writer (KeyfileWriterInfo *info, data = g_bytes_get_data (array, &len); if (!data || !len) return; - - tmp_array = g_new (gint, len); - for (i = 0; i < len; i++) - tmp_array[i] = (int) data[i]; - nm_keyfile_plugin_kf_set_integer_list (info->keyfile, setting_name, key, tmp_array, len); - g_free (tmp_array); + nm_keyfile_plugin_kf_set_integer_list_uint8 (info->keyfile, setting_name, key, data, len); } typedef struct ObjectType { @@ -729,17 +719,8 @@ write_setting_value (NMSetting *setting, bytes = g_value_get_boxed (value); data = bytes ? g_bytes_get_data (bytes, &len) : NULL; - if (data != NULL && len > 0) { - int *tmp_array; - int i; - - tmp_array = g_new (gint, len); - for (i = 0; i < len; i++) - tmp_array[i] = (int) data[i]; - - nm_keyfile_plugin_kf_set_integer_list (info->keyfile, setting_name, key, tmp_array, len); - g_free (tmp_array); - } + if (data != NULL && len > 0) + nm_keyfile_plugin_kf_set_integer_list_uint8 (info->keyfile, setting_name, key, data, len); } else if (type == G_TYPE_STRV) { char **array; From 932da77b5be51ef63216d46255ad71863cf8ce89 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 21 Dec 2016 20:10:03 +0100 Subject: [PATCH 7/8] keyfile: assert that write_array_of_uint() writes valid integer list We use write_array_of_uint() for G_TYPE_ARRAY. In practice, only NMSettingDcb has any properties of this type. Furthermore, all valid values are either gboolean or guints of restricted range. Thus, no valid NMSettingDcb should violate the range check. Same for reader. It's really ugly to blindly use uint-list reader for G_TYPE_ARRAY. Especially, because certain G_TYPE_ARRAY properties of NMSettingDcb are actually arrays of gboolean, which only ~accidentally~ has the same memory layout as guint. --- libnm-core/nm-keyfile-reader.c | 15 ++++++++++----- libnm-core/nm-keyfile-writer.c | 16 +++++++++++----- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/libnm-core/nm-keyfile-reader.c b/libnm-core/nm-keyfile-reader.c index 15d8c8895..4403b8a25 100644 --- a/libnm-core/nm-keyfile-reader.c +++ b/libnm-core/nm-keyfile-reader.c @@ -108,19 +108,24 @@ read_array_of_uint (GKeyFile *file, NMSetting *setting, const char *key) { - GArray *array = NULL; + gs_unref_array GArray *array = NULL; gsize length; - int i; + gsize i; gs_free int *tmp = NULL; tmp = nm_keyfile_plugin_kf_get_integer_list (file, nm_setting_get_name (setting), key, &length, NULL); - array = g_array_sized_new (FALSE, FALSE, sizeof (guint32), length); + if (length > G_MAXUINT) + return; - for (i = 0; i < length; i++) + array = g_array_sized_new (FALSE, FALSE, sizeof (guint), length); + + for (i = 0; i < length; i++) { + if (tmp[i] < 0) + return; g_array_append_val (array, tmp[i]); + } g_object_set (setting, key, array, NULL); - g_array_unref (array); } static gboolean diff --git a/libnm-core/nm-keyfile-writer.c b/libnm-core/nm-keyfile-writer.c index d0e3effb4..0a54d4614 100644 --- a/libnm-core/nm-keyfile-writer.c +++ b/libnm-core/nm-keyfile-writer.c @@ -73,19 +73,25 @@ write_array_of_uint (GKeyFile *file, const GValue *value) { GArray *array; - int i; - int *tmp_array; + guint i; + gs_free int *tmp_array = NULL; array = (GArray *) g_value_get_boxed (value); if (!array || !array->len) return; + g_return_if_fail (g_array_get_element_size (array) == sizeof (guint)); + tmp_array = g_new (gint, array->len); - for (i = 0; i < array->len; i++) - tmp_array[i] = g_array_index (array, int, i); + for (i = 0; i < array->len; i++) { + guint v = g_array_index (array, guint, i); + + if (v > G_MAXINT) + g_return_if_reached (); + tmp_array[i] = (int) v; + } nm_keyfile_plugin_kf_set_integer_list (file, nm_setting_get_name (setting), key, tmp_array, array->len); - g_free (tmp_array); } static void From e844df1099f5f12a8487a80203a87bfcba14c615 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Jan 2017 12:07:33 +0100 Subject: [PATCH 8/8] keyfile: write also empty byte arrays to keyfiles It's not the job of keyfile writer to enforce certain settings. A %NULL GBytes property is shall be treated distinct from a byte array with zero length. The NMSetting may or may not reject such settings as invalid during verify() or mangle them during normalize(). But reader/writer should just serialize every property as-is. --- libnm-core/nm-keyfile-writer.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/libnm-core/nm-keyfile-writer.c b/libnm-core/nm-keyfile-writer.c index 0a54d4614..92869a2c0 100644 --- a/libnm-core/nm-keyfile-writer.c +++ b/libnm-core/nm-keyfile-writer.c @@ -300,9 +300,8 @@ ssid_writer (KeyfileWriterInfo *info, gsize ssid_len; const char *setting_name = nm_setting_get_name (setting); gboolean new_format = TRUE; - unsigned int semicolons = 0; + gsize semicolons = 0; gsize i; - char *ssid; g_return_if_fail (G_VALUE_HOLDS (value, G_TYPE_BYTES)); @@ -310,14 +309,17 @@ ssid_writer (KeyfileWriterInfo *info, if (!bytes) return; ssid_data = g_bytes_get_data (bytes, &ssid_len); - if (ssid_len == 0) + if (!ssid_data || !ssid_len) { + nm_keyfile_plugin_kf_set_string (info->keyfile, setting_name, key, ""); return; + } /* Check whether each byte is printable. If not, we have to use an * integer list, otherwise we can just use a string. */ for (i = 0; i < ssid_len; i++) { - char c = ssid_data[i] & 0xFF; + const char c = ssid_data[i]; + if (!g_ascii_isprint (c)) { new_format = FALSE; break; @@ -327,22 +329,24 @@ ssid_writer (KeyfileWriterInfo *info, } if (new_format) { - ssid = g_malloc0 (ssid_len + semicolons + 1); + gs_free char *ssid = NULL; + if (semicolons == 0) - memcpy (ssid, ssid_data, ssid_len); + ssid = g_strndup ((char *) ssid_data, ssid_len); else { /* Escape semicolons with backslashes to make strings * containing ';', such as '16;17;' unambiguous */ gsize j = 0; + ssid = g_malloc (ssid_len + semicolons + 1); for (i = 0; i < ssid_len; i++) { if (ssid_data[i] == ';') ssid[j++] = '\\'; ssid[j++] = ssid_data[i]; } + ssid[j] = '\0'; } nm_keyfile_plugin_kf_set_string (info->keyfile, setting_name, key, ssid); - g_free (ssid); } else nm_keyfile_plugin_kf_set_integer_list_uint8 (info->keyfile, setting_name, key, ssid_data, ssid_len); } @@ -364,8 +368,8 @@ password_raw_writer (KeyfileWriterInfo *info, if (!array) return; data = g_bytes_get_data (array, &len); - if (!data || !len) - return; + if (!data) + len = 0; nm_keyfile_plugin_kf_set_integer_list_uint8 (info->keyfile, setting_name, key, data, len); }