From 8da91cd85f0e21d1c8c9d89d20a9e5ab5d7b03ac Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 14:05:53 +0200 Subject: [PATCH 01/16] glib-aux: add nm_clear_g_string() helper Since g_string_free() takes an additional argument, it's not direclty usable with nm_clear_pointer(ptr, g_string_free); As workaround, add nm_clear_g_string() helper. --- src/libnm-glib-aux/nm-macros-internal.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/libnm-glib-aux/nm-macros-internal.h b/src/libnm-glib-aux/nm-macros-internal.h index edaa6f3902..39197ecf3a 100644 --- a/src/libnm-glib-aux/nm-macros-internal.h +++ b/src/libnm-glib-aux/nm-macros-internal.h @@ -1136,6 +1136,18 @@ nm_clear_g_variant(GVariant **variant) return FALSE; } +static inline gboolean +nm_clear_g_string(GString **ptr) +{ + GString *s; + + if (ptr && (s = *ptr)) { + *ptr = NULL; + g_string_free(s, TRUE); + }; + return FALSE; +} + static inline gboolean nm_clear_g_cancellable(GCancellable **cancellable) { From 62c1944e7dcc22adbeb5b25bb9bbd268a1cccf16 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 14:14:26 +0200 Subject: [PATCH 02/16] libnm: fix logic and double free in nm_vpn_service_plugin_read_vpn_details() "val" and "key" are now marked as nm_auto. They are freed at the end, and we should not free them before breaking the loop (at least not, without also clearing the variables). Fixes: 02dbba49d6d3 ('libnm: fix leak in nm_vpn_service_plugin_read_vpn_details()') --- src/libnm-client-impl/nm-vpn-service-plugin.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/libnm-client-impl/nm-vpn-service-plugin.c b/src/libnm-client-impl/nm-vpn-service-plugin.c index 016c761c40..1188924afc 100644 --- a/src/libnm-client-impl/nm-vpn-service-plugin.c +++ b/src/libnm-client-impl/nm-vpn-service-plugin.c @@ -759,8 +759,7 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable nm_auto_free_gstring GString *val = NULL; nm_auto_free_gstring GString *line = NULL; char c; - - GString *str = NULL; + GString * str = NULL; if (out_data) g_return_val_if_fail(*out_data == NULL, FALSE); @@ -808,16 +807,12 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable /* finish marker */ break; } else if (strncmp(line->str, DATA_KEY_TAG, strlen(DATA_KEY_TAG)) == 0) { - if (key != NULL) { + if (nm_clear_g_string(&key)) g_warning("a value expected"); - g_string_free(key, TRUE); - } key = g_string_new(line->str + strlen(DATA_KEY_TAG)); str = key; hash = data; } else if (strncmp(line->str, DATA_VAL_TAG, strlen(DATA_VAL_TAG)) == 0) { - if (val != NULL) - g_string_free(val, TRUE); if (val || !key || hash != data) { g_warning("%s not preceded by %s", DATA_VAL_TAG, DATA_KEY_TAG); break; @@ -825,16 +820,12 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable val = g_string_new(line->str + strlen(DATA_VAL_TAG)); str = val; } else if (strncmp(line->str, SECRET_KEY_TAG, strlen(SECRET_KEY_TAG)) == 0) { - if (key != NULL) { + if (nm_clear_g_string(&key)) g_warning("a value expected"); - g_string_free(key, TRUE); - } key = g_string_new(line->str + strlen(SECRET_KEY_TAG)); str = key; hash = secrets; } else if (strncmp(line->str, SECRET_VAL_TAG, strlen(SECRET_VAL_TAG)) == 0) { - if (val != NULL) - g_string_free(val, TRUE); if (val || !key || hash != secrets) { g_warning("%s not preceded by %s", SECRET_VAL_TAG, SECRET_KEY_TAG); break; From f0dc95e51759518f87f2603014a30338ad36d0ab Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 14:17:41 +0200 Subject: [PATCH 03/16] libnm: avoid strcmp in nm_vpn_service_plugin_read_vpn_details() --- src/libnm-client-impl/nm-vpn-service-plugin.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libnm-client-impl/nm-vpn-service-plugin.c b/src/libnm-client-impl/nm-vpn-service-plugin.c index 1188924afc..5c7bc45b6d 100644 --- a/src/libnm-client-impl/nm-vpn-service-plugin.c +++ b/src/libnm-client-impl/nm-vpn-service-plugin.c @@ -803,29 +803,29 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable success = TRUE; /* Got at least one value */ } - if (strcmp(line->str, "DONE") == 0) { + if (nm_streq(line->str, "DONE")) { /* finish marker */ break; - } else if (strncmp(line->str, DATA_KEY_TAG, strlen(DATA_KEY_TAG)) == 0) { + } else if (NM_STR_HAS_PREFIX(line->str, DATA_KEY_TAG)) { if (nm_clear_g_string(&key)) g_warning("a value expected"); key = g_string_new(line->str + strlen(DATA_KEY_TAG)); str = key; hash = data; - } else if (strncmp(line->str, DATA_VAL_TAG, strlen(DATA_VAL_TAG)) == 0) { + } else if (NM_STR_HAS_PREFIX(line->str, DATA_VAL_TAG)) { if (val || !key || hash != data) { g_warning("%s not preceded by %s", DATA_VAL_TAG, DATA_KEY_TAG); break; } val = g_string_new(line->str + strlen(DATA_VAL_TAG)); str = val; - } else if (strncmp(line->str, SECRET_KEY_TAG, strlen(SECRET_KEY_TAG)) == 0) { + } else if (NM_STR_HAS_PREFIX(line->str, SECRET_KEY_TAG)) { if (nm_clear_g_string(&key)) g_warning("a value expected"); key = g_string_new(line->str + strlen(SECRET_KEY_TAG)); str = key; hash = secrets; - } else if (strncmp(line->str, SECRET_VAL_TAG, strlen(SECRET_VAL_TAG)) == 0) { + } else if (NM_STR_HAS_PREFIX(line->str, SECRET_VAL_TAG)) { if (val || !key || hash != secrets) { g_warning("%s not preceded by %s", SECRET_VAL_TAG, SECRET_KEY_TAG); break; From ddf1942bfb640ddcfbcd663f21b90fd39eb2ac29 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 14:18:31 +0200 Subject: [PATCH 04/16] libnm: avoid g_warning() in nm_vpn_service_plugin_read_vpn_details() g_warning() and printing to stdout/stderr are not suitable actions for a library. If there is something important, find a way to report the condition to the caller. If it's not important, stay quiet. --- src/libnm-client-impl/nm-vpn-service-plugin.c | 14 ++++---------- src/libnm-client-impl/tests/test-libnm.c | 3 +-- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/src/libnm-client-impl/nm-vpn-service-plugin.c b/src/libnm-client-impl/nm-vpn-service-plugin.c index 5c7bc45b6d..7ed0c5fabe 100644 --- a/src/libnm-client-impl/nm-vpn-service-plugin.c +++ b/src/libnm-client-impl/nm-vpn-service-plugin.c @@ -807,29 +807,23 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable /* finish marker */ break; } else if (NM_STR_HAS_PREFIX(line->str, DATA_KEY_TAG)) { - if (nm_clear_g_string(&key)) - g_warning("a value expected"); + nm_clear_g_string(&key); key = g_string_new(line->str + strlen(DATA_KEY_TAG)); str = key; hash = data; } else if (NM_STR_HAS_PREFIX(line->str, DATA_VAL_TAG)) { - if (val || !key || hash != data) { - g_warning("%s not preceded by %s", DATA_VAL_TAG, DATA_KEY_TAG); + if (val || !key || hash != data) break; - } val = g_string_new(line->str + strlen(DATA_VAL_TAG)); str = val; } else if (NM_STR_HAS_PREFIX(line->str, SECRET_KEY_TAG)) { - if (nm_clear_g_string(&key)) - g_warning("a value expected"); + nm_clear_g_string(&key); key = g_string_new(line->str + strlen(SECRET_KEY_TAG)); str = key; hash = secrets; } else if (NM_STR_HAS_PREFIX(line->str, SECRET_VAL_TAG)) { - if (val || !key || hash != secrets) { - g_warning("%s not preceded by %s", SECRET_VAL_TAG, SECRET_KEY_TAG); + if (val || !key || hash != secrets) break; - } val = g_string_new(line->str + strlen(SECRET_VAL_TAG)); str = val; } diff --git a/src/libnm-client-impl/tests/test-libnm.c b/src/libnm-client-impl/tests/test-libnm.c index 003b030432..d09feb5ce3 100644 --- a/src/libnm-client-impl/tests/test-libnm.c +++ b/src/libnm-client-impl/tests/test-libnm.c @@ -2462,8 +2462,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "DONE\n" "", READ_VPN_DETAIL_DATA({"some-key", "string\ncontinued after a line break"}, ), - READ_VPN_DETAIL_DATA({"key names\ncan have\ncontinuations too", "value"}, ), - NMTST_EXPECT_LIBNM_WARNING("DATA_VAL= not preceded by DATA_KEY=")); + READ_VPN_DETAIL_DATA({"key names\ncan have\ncontinuations too", "value"}, ), ); _do_read_vpn_details( "" From 1338a2ef96b873415b0a2f2429b504b5d40cf087 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 14:23:10 +0200 Subject: [PATCH 05/16] libnm: avoid sleep in nm_vpn_service_plugin_read_vpn_details() Polling with sleep() is really ugly. Use poll() instead. --- src/libnm-client-impl/nm-vpn-service-plugin.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/libnm-client-impl/nm-vpn-service-plugin.c b/src/libnm-client-impl/nm-vpn-service-plugin.c index 7ed0c5fabe..8a49260599 100644 --- a/src/libnm-client-impl/nm-vpn-service-plugin.c +++ b/src/libnm-client-impl/nm-vpn-service-plugin.c @@ -10,6 +10,7 @@ #include #include +#include #include "libnm-glib-aux/nm-secret-utils.h" #include "libnm-glib-aux/nm-dbus-aux.h" @@ -779,8 +780,18 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable nr = read(fd, &c, 1); if (nr < 0) { if (errno == EAGAIN) { - g_usleep(100); - continue; + struct pollfd pfd; + int r; + + memset(&pfd, 0, sizeof(pfd)); + pfd.fd = fd; + pfd.events = POLLIN; + + r = poll(&pfd, 1, -1); + if (r > 0) + continue; + + /* error or timeout. Fall through and break. */ } break; } From 4a9fcb0fc32ed67646843312a6cbc706ec3fee45 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 14:23:10 +0200 Subject: [PATCH 06/16] libnm: buffer reads in nm_vpn_service_plugin_read_vpn_details() It seems very ugly to read one byte at a time. Use a naive buffered reader, so that we can read multiple bytes at a time, and return them one by one. Also, this now keeps state of any error/EOF. Once we reach EOF, we won't read again. The previous code did that too, but I think this code is easier to read. --- src/libnm-client-impl/nm-vpn-service-plugin.c | 82 ++++++++++++++----- 1 file changed, 60 insertions(+), 22 deletions(-) diff --git a/src/libnm-client-impl/nm-vpn-service-plugin.c b/src/libnm-client-impl/nm-vpn-service-plugin.c index 8a49260599..5480b6538f 100644 --- a/src/libnm-client-impl/nm-vpn-service-plugin.c +++ b/src/libnm-client-impl/nm-vpn-service-plugin.c @@ -729,6 +729,54 @@ nm_vpn_service_plugin_secrets_required(NMVpnServicePlugin *plugin, /*****************************************************************************/ +typedef struct { + char *buf; + gsize n_buf; + int fd; + bool eof : 1; + char buf_full[1024]; +} ReadFdBuf; + +static inline gboolean +_read_fd_buf_c(ReadFdBuf *read_buf, char *ch) +{ + gssize n_read; + + if (read_buf->n_buf > 0) + goto out_data; + if (read_buf->eof) + return FALSE; + +again: + n_read = read(read_buf->fd, read_buf->buf_full, sizeof(read_buf->buf_full)); + if (n_read <= 0) { + if (n_read < 0 && errno == EAGAIN) { + struct pollfd pfd; + int r; + + memset(&pfd, 0, sizeof(pfd)); + pfd.fd = read_buf->fd; + pfd.events = POLLIN; + + r = poll(&pfd, 1, -1); + if (r > 0) + goto again; + /* error or timeout. Fall through and set EOF. */ + } + read_buf->eof = TRUE; + return FALSE; + } + + read_buf->buf = read_buf->buf_full; + read_buf->n_buf = n_read; + +out_data: + read_buf->n_buf--; + *ch = read_buf->buf[0]; + read_buf->buf++; + return TRUE; +} + #define DATA_KEY_TAG "DATA_KEY=" #define DATA_VAL_TAG "DATA_VAL=" #define SECRET_KEY_TAG "SECRET_KEY=" @@ -759,8 +807,8 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable nm_auto_free_gstring GString *key = NULL; nm_auto_free_gstring GString *val = NULL; nm_auto_free_gstring GString *line = NULL; - char c; - GString * str = NULL; + GString * str = NULL; + ReadFdBuf read_buf; if (out_data) g_return_val_if_fail(*out_data == NULL, FALSE); @@ -771,31 +819,21 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable secrets = g_hash_table_new_full(nm_str_hash, g_str_equal, g_free, (GDestroyNotify) nm_free_secret); + read_buf.buf = NULL; + read_buf.n_buf = 0; + read_buf.fd = fd; + read_buf.eof = FALSE; + line = g_string_new(NULL); /* Read stdin for data and secret items until we get a DONE */ while (1) { - ssize_t nr; + gboolean eof; + char c; - nr = read(fd, &c, 1); - if (nr < 0) { - if (errno == EAGAIN) { - struct pollfd pfd; - int r; + eof = !_read_fd_buf_c(&read_buf, &c); - memset(&pfd, 0, sizeof(pfd)); - pfd.fd = fd; - pfd.events = POLLIN; - - r = poll(&pfd, 1, -1); - if (r > 0) - continue; - - /* error or timeout. Fall through and break. */ - } - break; - } - if (nr > 0 && c != '\n') { + if (!eof && c != '\n') { g_string_append_c(line, c); continue; } @@ -841,7 +879,7 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable g_string_truncate(line, 0); - if (nr == 0) + if (eof) break; } From 6bf7908d05d15eb62e61435f76ddae1f78dcdb7b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 14:23:10 +0200 Subject: [PATCH 07/16] libnm: abort huge read in nm_vpn_service_plugin_read_vpn_details() There is no need to accept such a huge read. Abort. --- src/libnm-client-impl/nm-vpn-service-plugin.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libnm-client-impl/nm-vpn-service-plugin.c b/src/libnm-client-impl/nm-vpn-service-plugin.c index 5480b6538f..63dbbbbefc 100644 --- a/src/libnm-client-impl/nm-vpn-service-plugin.c +++ b/src/libnm-client-impl/nm-vpn-service-plugin.c @@ -835,6 +835,10 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable if (!eof && c != '\n') { g_string_append_c(line, c); + if (line->len > 512 * 1024) { + /* we are about to read a huge line. That is not right, abort. */ + break; + } continue; } From f305a411cf3e7cbe013809346b5aed4ce1f24167 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 14:23:10 +0200 Subject: [PATCH 08/16] libnm: abort read in nm_vpn_service_plugin_read_vpn_details() on '\0' We expect to read NUL terminated strings. Upon NUL, we should do something. Assume this is EOF. --- src/libnm-client-impl/nm-vpn-service-plugin.c | 14 ++- src/libnm-client-impl/tests/test-libnm.c | 108 ++++++++++++------ 2 files changed, 83 insertions(+), 39 deletions(-) diff --git a/src/libnm-client-impl/nm-vpn-service-plugin.c b/src/libnm-client-impl/nm-vpn-service-plugin.c index 63dbbbbefc..18a85e913b 100644 --- a/src/libnm-client-impl/nm-vpn-service-plugin.c +++ b/src/libnm-client-impl/nm-vpn-service-plugin.c @@ -829,10 +829,16 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable /* Read stdin for data and secret items until we get a DONE */ while (1) { gboolean eof; - char c; + char c = '\0'; eof = !_read_fd_buf_c(&read_buf, &c); + if (!eof && c == '\0') { + /* On the first '\0' char, we also assume the data is finished. Abort. */ + read_buf.eof = TRUE; + eof = TRUE; + } + if (!eof && c != '\n') { g_string_append_c(line, c); if (line->len > 512 * 1024) { @@ -846,7 +852,10 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable /* continuation */ g_string_append_c(str, '\n'); g_string_append(str, line->str + 1); - } else if (key && val) { + goto next; + } + + if (key && val) { /* done a line */ g_return_val_if_fail(hash, FALSE); g_hash_table_insert(hash, g_string_free(key, FALSE), g_string_free(val, FALSE)); @@ -881,6 +890,7 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable str = val; } +next: g_string_truncate(line, 0); if (eof) diff --git a/src/libnm-client-impl/tests/test-libnm.c b/src/libnm-client-impl/tests/test-libnm.c index d09feb5ce3..6cb8076e38 100644 --- a/src/libnm-client-impl/tests/test-libnm.c +++ b/src/libnm-client-impl/tests/test-libnm.c @@ -2477,43 +2477,77 @@ test_nm_vpn_service_plugin_read_vpn_details(void) READ_VPN_DETAIL_DATA({"some-key", "string\ncontinued after a line break"}, ), READ_VPN_DETAIL_DATA({"key names\ncan have\ncontinuations too", "value"}, ), ); - _do_read_vpn_details( - "" - "DATA_KEY=some-key\n" - "DATA_VAL=string\n" - "\n" - "DATA_KEY=some\n" - "=key-2\n" - "DATA_VAL=val2\n" - "\n" - "DATA_KEY=key3\0" - "=key-2\n" - "DATA_VAL=val3\n" - "\n" - "SECRET_KEY=some-secret\n" - "SECRET_VAL=val3\n" - "\n" - "SECRET_KEY=\n" - "SECRET_VAL=val3\n" - "\n" - "SECRET_KEY=keyx\n" - "SECRET_VAL=\n" - "\n" - "SECRET_KEY=ke\xc0yx\n" - "SECRET_VAL=inval\n" - "\n" - "SECRET_KEY=key-inval\n" - "SECRET_VAL=in\xc1val\n" - "\n" - "DONE\n" - "\n" - "", - READ_VPN_DETAIL_DATA({"some\nkey-2", "val2"}, {"some-key", "string"}, {"key3", "val3"}, ), - READ_VPN_DETAIL_DATA({"some-secret", "val3"}, - {"", "val3"}, - {"keyx", ""}, - {"ke\xc0yx", "inval"}, - {"key-inval", "in\xc1val"}, ), ); + _do_read_vpn_details("" + "DATA_KEY=some-key\n" + "DATA_VAL=string\n" + "\n" + "DATA_KEY=some\n" + "=key-2\n" + "DATA_VAL=val2\n" + "\n" + "DATA_KEY=key3\n" + "=key-2\n" + "DATA_VAL=val3\n" + "\n" + "SECRET_KEY=some-secret\n" + "SECRET_VAL=val3\n" + "\n" + "SECRET_KEY=\n" + "SECRET_VAL=val3\n" + "\n" + "SECRET_KEY=keyx\n" + "SECRET_VAL=\n" + "\n" + "SECRET_KEY=ke\xc0yx\n" + "SECRET_VAL=inval\n" + "\n" + "SECRET_KEY=key-inval\n" + "SECRET_VAL=in\xc1val\n" + "\n" + "DONE\n" + "\n" + "", + READ_VPN_DETAIL_DATA({"some\nkey-2", "val2"}, + {"some-key", "string"}, + {"key3\nkey-2", "val3"}, ), + READ_VPN_DETAIL_DATA({"some-secret", "val3"}, + {"", "val3"}, + {"keyx", ""}, + {"ke\xc0yx", "inval"}, + {"key-inval", "in\xc1val"}, ), ); + + _do_read_vpn_details("" + "DATA_KEY=some-key\n" + "DATA_VAL=string\n" + "\n" + "DATA_KEY=some\n" + "=key-2\n" + "DATA_VAL=val2\n" + "\n" + "DATA_KEY=key3\0" + "=key-2\n" + "DATA_VAL=val3\n" + "\n" + "SECRET_KEY=some-secret\n" + "SECRET_VAL=val3\n" + "\n" + "SECRET_KEY=\n" + "SECRET_VAL=val3\n" + "\n" + "SECRET_KEY=keyx\n" + "SECRET_VAL=\n" + "\n" + "SECRET_KEY=ke\xc0yx\n" + "SECRET_VAL=inval\n" + "\n" + "SECRET_KEY=key-inval\n" + "SECRET_VAL=in\xc1val\n" + "\n" + "DONE\n" + "\n" + "", + READ_VPN_DETAIL_DATA({"some\nkey-2", "val2"}, {"some-key", "string"}, ), + READ_VPN_DETAIL_DATA(), ); } /*****************************************************************************/ From 370316fc3e5831227507689462a59bef56c2bd39 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 17:35:50 +0200 Subject: [PATCH 09/16] ifcfg-rh: allocate exact buffer in _escape_ansic() Previously, we would allocate a buffer of the worst case, that is, 4 times the number of bytes, in case all of them require octal escaping. Coverity doesn't like _escape_ansic() for another reason: Error: NULL_RETURNS (CWE-476): [#def298] NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:161: returned_null: "g_malloc" returns "NULL". NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:161: alias: Assigning: "q" = "dest = g_malloc(strlen(source) * 4UL + 1UL + 3UL)". Both pointers are now "NULL". NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:163: dereference: Incrementing a pointer which might be null: "q". # 161| q = dest = g_malloc(strlen(source) * 4 + 1 + 3); # 162| # 163|-> *q++ = '$'; # 164| *q++ = '\''; # 165| It doesn't recognize that g_malloc() shouldn't return NULL (because we never request zero bytes). I am not sure how to avoid that, but let's rework the code to first count how many characters we exactly need. It think that should also help with the coverity warning. Doing exact allocation requires first to count the number of required bytes. It still might be worth it, because we might keep the allocated strings a bit longer around. --- src/core/settings/plugins/ifcfg-rh/shvar.c | 36 ++++++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/src/core/settings/plugins/ifcfg-rh/shvar.c b/src/core/settings/plugins/ifcfg-rh/shvar.c index 371756e8c3..d120251b0c 100644 --- a/src/core/settings/plugins/ifcfg-rh/shvar.c +++ b/src/core/settings/plugins/ifcfg-rh/shvar.c @@ -153,17 +153,42 @@ _escape_ansic(const char *source) const char *p; char * dest; char * q; + gsize n_alloc; nm_assert(source); - p = (const char *) source; - /* Each source byte needs maximally four destination chars (\777) */ - q = dest = g_malloc(strlen(source) * 4 + 1 + 3); + n_alloc = 4; + for (p = source; p[0]; p++) { + switch (*p) { + case '\b': + case '\f': + case '\n': + case '\r': + case '\t': + case '\v': + case '\\': + case '"': + case '\'': + n_alloc += 2; + break; + default: + if ((*p < ' ') || (*p >= 0177)) + n_alloc += 4; + else + n_alloc += 1; + break; + } + } + + dest = g_malloc(n_alloc); + + q = dest; *q++ = '$'; *q++ = '\''; - while (*p) { + for (p = source; p[0]; p++) { + nm_assert(q < &dest[n_alloc]); switch (*p) { case '\b': *q++ = '\\'; @@ -205,12 +230,11 @@ _escape_ansic(const char *source) *q++ = *p; break; } - p++; } *q++ = '\''; *q++ = '\0'; - nm_assert(q - dest <= strlen(source) * 4 + 1 + 3); + nm_assert(q - dest == n_alloc); return dest; } From 8db23d47e40a2ea9926eb62b53bc0b169dfea612 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 18:35:25 +0200 Subject: [PATCH 10/16] ifcfg-rh: minor cleanup in svEscape() --- src/core/settings/plugins/ifcfg-rh/shvar.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/core/settings/plugins/ifcfg-rh/shvar.c b/src/core/settings/plugins/ifcfg-rh/shvar.c index d120251b0c..80644b64fd 100644 --- a/src/core/settings/plugins/ifcfg-rh/shvar.c +++ b/src/core/settings/plugins/ifcfg-rh/shvar.c @@ -242,7 +242,7 @@ _escape_ansic(const char *source) /*****************************************************************************/ #define _char_req_escape(ch) NM_IN_SET(ch, '"', '\\', '$', '`') -#define _char_req_escape_old(ch) NM_IN_SET(ch, '"', '\\', '\'', '$', '`', '~') +#define _char_req_escape_old(ch) NM_IN_SET(ch, '"', '\\', '$', '`', '\'', '~') #define _char_req_quotes(ch) NM_IN_SET(ch, ' ', '\'', '~', '\t', '|', '&', ';', '(', ')', '<', '>') const char * @@ -251,8 +251,10 @@ svEscape(const char *s, char **to_free) char *new; gsize mangle = 0; gboolean requires_quotes = FALSE; - int newlen; - size_t i, j, slen; + gsize n_alloc; + gsize slen; + gsize i; + gsize j; for (slen = 0; s[slen]; slen++) { if (_char_req_escape(s[slen])) @@ -272,8 +274,8 @@ svEscape(const char *s, char **to_free) return s; } - newlen = slen + mangle + 3; /* 3 is extra ""\0 */ - new = g_malloc(newlen); + n_alloc = slen + mangle + 3; /* 3 is extra ""\0 */ + new = g_malloc(n_alloc); j = 0; new[j++] = '"'; @@ -285,7 +287,7 @@ svEscape(const char *s, char **to_free) new[j++] = '"'; new[j++] = '\0'; - nm_assert(j == slen + mangle + 3); + nm_assert(j == n_alloc); *to_free = new; return new; From e56f126071882b88f3d6ef53cb611794d12e5e14 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 21:50:41 +0200 Subject: [PATCH 11/16] libnm: fix error handling in NMVpnPluginOld's _connect_generic() Also Coverity found that something is wrong here: Error: FORWARD_NULL (CWE-476): [#def361] NetworkManager-1.31.5/src/libnm-client-impl/nm-vpn-plugin-old.c:441: var_compare_op: Comparing "connection" to null implies that "connection" might be null. NetworkManager-1.31.5/src/libnm-client-impl/nm-vpn-plugin-old.c:489: var_deref_model: Passing null pointer "connection" to "g_object_unref", which dereferences it. # 487| } # 488| # 489|-> g_object_unref(connection); # 490| } # 491| Fixes: 6793a32a8c54 ('libnm: port to GDBus') --- src/libnm-client-impl/nm-vpn-plugin-old.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/libnm-client-impl/nm-vpn-plugin-old.c b/src/libnm-client-impl/nm-vpn-plugin-old.c index be4e1cf292..7430622b90 100644 --- a/src/libnm-client-impl/nm-vpn-plugin-old.c +++ b/src/libnm-client-impl/nm-vpn-plugin-old.c @@ -420,12 +420,12 @@ _connect_generic(NMVpnPluginOld * plugin, GVariant * properties, GVariant * details) { - NMVpnPluginOldPrivate *priv = NM_VPN_PLUGIN_OLD_GET_PRIVATE(plugin); - NMVpnPluginOldClass * vpn_class = NM_VPN_PLUGIN_OLD_GET_CLASS(plugin); - NMConnection * connection; - gboolean success = FALSE; - GError * error = NULL; - guint fail_stop_timeout = 0; + NMVpnPluginOldPrivate *priv = NM_VPN_PLUGIN_OLD_GET_PRIVATE(plugin); + NMVpnPluginOldClass * vpn_class = NM_VPN_PLUGIN_OLD_GET_CLASS(plugin); + gs_unref_object NMConnection *connection = NULL; + gboolean success = FALSE; + GError * error = NULL; + guint fail_stop_timeout = 0; if (priv->state != NM_VPN_SERVICE_STATE_STOPPED && priv->state != NM_VPN_SERVICE_STATE_INIT) { g_dbus_method_invocation_return_error(context, @@ -445,6 +445,7 @@ _connect_generic(NMVpnPluginOld * plugin, "Invalid connection: %s", error->message); g_clear_error(&error); + return; } priv->interactive = FALSE; @@ -485,8 +486,6 @@ _connect_generic(NMVpnPluginOld * plugin, */ schedule_fail_stop(plugin, fail_stop_timeout); } - - g_object_unref(connection); } static void From bbe39ed095c8d7b2158ec4454cc6dd73d25c818a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 21:53:46 +0200 Subject: [PATCH 12/16] libnm: use cleanup attribute in NMVpnPluginOld's _connect_generic() --- src/libnm-client-impl/nm-vpn-plugin-old.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/libnm-client-impl/nm-vpn-plugin-old.c b/src/libnm-client-impl/nm-vpn-plugin-old.c index 7430622b90..f60bf19c7b 100644 --- a/src/libnm-client-impl/nm-vpn-plugin-old.c +++ b/src/libnm-client-impl/nm-vpn-plugin-old.c @@ -420,12 +420,12 @@ _connect_generic(NMVpnPluginOld * plugin, GVariant * properties, GVariant * details) { - NMVpnPluginOldPrivate *priv = NM_VPN_PLUGIN_OLD_GET_PRIVATE(plugin); - NMVpnPluginOldClass * vpn_class = NM_VPN_PLUGIN_OLD_GET_CLASS(plugin); - gs_unref_object NMConnection *connection = NULL; - gboolean success = FALSE; - GError * error = NULL; - guint fail_stop_timeout = 0; + NMVpnPluginOldPrivate *priv = NM_VPN_PLUGIN_OLD_GET_PRIVATE(plugin); + NMVpnPluginOldClass * vpn_class = NM_VPN_PLUGIN_OLD_GET_CLASS(plugin); + gs_unref_object NMConnection *connection = NULL; + gboolean success = FALSE; + gs_free_error GError *error = NULL; + guint fail_stop_timeout = 0; if (priv->state != NM_VPN_SERVICE_STATE_STOPPED && priv->state != NM_VPN_SERVICE_STATE_INIT) { g_dbus_method_invocation_return_error(context, @@ -444,7 +444,6 @@ _connect_generic(NMVpnPluginOld * plugin, NM_VPN_PLUGIN_ERROR_BAD_ARGUMENTS, "Invalid connection: %s", error->message); - g_clear_error(&error); return; } @@ -479,7 +478,7 @@ _connect_generic(NMVpnPluginOld * plugin, /* Add a timer to make sure we do not wait indefinitely for the successful connect. */ connect_timer_start(plugin); } else { - g_dbus_method_invocation_take_error(context, error); + g_dbus_method_invocation_take_error(context, g_steal_pointer(&error)); /* Stop the plugin from an idle handler so that the Connect * method return gets sent before the STOP StateChanged signal. From 6646ee6546d03b9a3b15e7029dbaa69c82c08503 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 22:46:03 +0200 Subject: [PATCH 13/16] libnm/tests: avoid potential crash in test code test_nm_utils_escaped_tokens() It causes a Coverity warning, so let's work around it. --- src/libnm-core-impl/tests/test-general.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index 67b6ab482f..75146fcc60 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -1262,7 +1262,7 @@ test_nm_utils_escaped_tokens(void) num_options); g_print(">>> combined = \"%s\"\n", combined->str); g_print(">>> %c parsed[%5d].key = \"%s\"\n", - nm_streq(key, expected_key) ? ' ' : 'X', + nm_streq0(key, expected_key) ? ' ' : 'X', i_option, key); g_print(">>> %c parsed[%5d].val = %s%s%s\n", From a559950d4126a43d70d4384bc22348a4dd665ecd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 22:48:52 +0200 Subject: [PATCH 14/16] libnm/tests: avoid Coverity warning in test code _do_test_utils_str_utf8safe_unescape() Error: FORWARD_NULL (CWE-476): [#def435] NetworkManager-1.31.5/src/libnm-core-impl/tests/test-general.c:9084: var_compare_op: Comparing "str" to null implies that "str" might be null. NetworkManager-1.31.5/src/libnm-core-impl/tests/test-general.c:9105: var_deref_model: Passing null pointer "str" to "strchr", which dereferences it. # 9103| s = nm_utils_str_utf8safe_unescape(str, NM_UTILS_STR_UTF8_SAFE_FLAG_NONE, &str_free_1); # 9104| g_assert_cmpstr(s, ==, expected); # 9105|-> if (strchr(str, '\\')) { # 9106| g_assert(str_free_1 != str); # 9107| g_assert(s == str_free_1); --- src/libnm-core-impl/tests/test-general.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index 75146fcc60..c911f0ec2c 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -9102,6 +9102,7 @@ _do_test_utils_str_utf8safe_unescape(const char *str, const char *expected, gsiz /* there are no embedded NULs. Check that nm_utils_str_utf8safe_unescape() yields the same result. */ s = nm_utils_str_utf8safe_unescape(str, NM_UTILS_STR_UTF8_SAFE_FLAG_NONE, &str_free_1); g_assert_cmpstr(s, ==, expected); + g_assert(str); if (strchr(str, '\\')) { g_assert(str_free_1 != str); g_assert(s == str_free_1); From 9154f0128abe665f5a86a87130f5a7bae5991a53 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 May 2021 22:55:06 +0200 Subject: [PATCH 15/16] glib-aux: avoid coverity warning in nm_str_buf_append_printf() It's a false positive. Still avoid it. Error: FORWARD_NULL (CWE-476): [#def479] NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5558: var_compare_op: Comparing "strbuf->_priv_str" to null implies that "strbuf->_priv_str" might be null. NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5575: var_deref_model: Passing "strbuf" to "nm_str_buf_maybe_expand", which dereferences null "strbuf->_priv_str". # 5573| l2 = ((gsize) l) + 1u; # 5574| # 5575|-> nm_str_buf_maybe_expand(strbuf, l2, FALSE); # 5576| # 5577| va_start(args, format); Error: FORWARD_NULL (CWE-476): [#def480] NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5558: var_compare_op: Comparing "strbuf->_priv_str" to null implies that "strbuf->_priv_str" might be null. NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5575: no_write_call: Although "nm_str_buf_maybe_expand" does overwrite "strbuf->_priv_str" on some paths, it also contains at least one feasible path which does not overwrite it. NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5578: var_deref_op: Dereferencing null pointer "strbuf->_priv_str". # 5576| # 5577| va_start(args, format); # 5578|-> l = g_vsnprintf(&strbuf->_priv_str[strbuf->_priv_len], l2, format, args); # 5579| va_end(args); # 5580| --- src/libnm-glib-aux/nm-shared-utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-shared-utils.c b/src/libnm-glib-aux/nm-shared-utils.c index 26856cd1ee..2a7432751d 100644 --- a/src/libnm-glib-aux/nm-shared-utils.c +++ b/src/libnm-glib-aux/nm-shared-utils.c @@ -5555,7 +5555,7 @@ nm_str_buf_append_printf(NMStrBuf *strbuf, const char *format, ...) nm_assert(available < G_MAXULONG); va_start(args, format); - l = g_vsnprintf(strbuf->_priv_str ? &strbuf->_priv_str[strbuf->_priv_len] : NULL, + l = g_vsnprintf(strbuf->_priv_allocated > 0 ? &strbuf->_priv_str[strbuf->_priv_len] : NULL, available, format, args); From 8fcbbdd7a4ddf6dac62e0cc87f6027cc16d3f080 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 26 May 2021 09:31:07 +0200 Subject: [PATCH 16/16] all: reimplement g_strstrip() macro to avoid Coverity warning Coverity has issues with functions that handle ownership like g_strstrip(). Thus the scan is full of false positives like: Error: RESOURCE_LEAK (CWE-772): [#def45] [important] NetworkManager-1.31.5/src/core/devices/wwan/nm-service-providers.c:134: alloc_fn: Storage is returned from allocation function "g_strdup". NetworkManager-1.31.5/src/core/devices/wwan/nm-service-providers.c:134: noescape: Resource "g_strdup(attribute_values[i])" is not freed or pointed-to in "g_strchug". NetworkManager-1.31.5/src/core/devices/wwan/nm-service-providers.c:134: leaked_storage: Failing to save or free storage allocated by "g_strdup(attribute_values[i])" leaks it. # 132| if (strcmp(attribute_names[i], "value") == 0) { # 133| parse_context->state = PARSER_METHOD_GSM_APN; # 134|-> parse_context->apn = g_strstrip(g_strdup(attribute_values[i])); # 135| break; # 136| } Add a workaround for that. There are other functions that have the same problem, but the usage g_strstrip(g_strdup(...)) is common to warrant a special workaround. --- src/libnm-glib-aux/nm-glib.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/libnm-glib-aux/nm-glib.h b/src/libnm-glib-aux/nm-glib.h index 44a7d19f82..66e5c16e14 100644 --- a/src/libnm-glib-aux/nm-glib.h +++ b/src/libnm-glib-aux/nm-glib.h @@ -693,4 +693,18 @@ _nm_g_cancellable_reset(GCancellable *cancellable); /*****************************************************************************/ +/* Coverity gets confused by g_strstrip(g_strdup(foo)). Reimplement the macro + * in a way that hopefully works better to avoid the false positive. */ +#undef g_strstrip +#define g_strstrip(str) \ + ({ \ + char *const _str = (str); \ + \ + g_strchug(_str); \ + g_strchomp(_str); \ + _str; \ + }) + +/*****************************************************************************/ + #endif /* __NM_GLIB_H__ */