From 78a5c72ed650813668d730dbb4dda3b132b87a96 Mon Sep 17 00:00:00 2001 From: Bryan Jacobs Date: Sun, 27 Mar 2022 13:07:29 +1100 Subject: [PATCH 1/3] Revert "libnm: buffer reads in nm_vpn_service_plugin_read_vpn_details()" This partially reverts commit 4a9fcb0fc32e, which replaced one-byte reads with buffered ones in the VPN service plugin. Unfortunately the buffering means that commands coming after the magic "DONE" string were being pulled into the buffer. Secrets agents expect a "QUIT" to come after the "DONE", and since with buffering "QUIT" was in the buffer, this led to a twenty-second delay on every VPN connection using a secrets manager. https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1164 Fixes: 4a9fcb0fc32e ('libnm: buffer reads in nm_vpn_service_plugin_read_vpn_details()') (cherry picked from commit 8ae9cf4698b4fadae8cfbfbc801cf93d2385629d) --- src/libnm-client-impl/nm-vpn-service-plugin.c | 84 +++++-------------- 1 file changed, 20 insertions(+), 64 deletions(-) diff --git a/src/libnm-client-impl/nm-vpn-service-plugin.c b/src/libnm-client-impl/nm-vpn-service-plugin.c index 2a217502de..7b04c30799 100644 --- a/src/libnm-client-impl/nm-vpn-service-plugin.c +++ b/src/libnm-client-impl/nm-vpn-service-plugin.c @@ -729,54 +729,6 @@ 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=" @@ -808,7 +760,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; GString *str = NULL; - ReadFdBuf read_buf; + char c; if (out_data) g_return_val_if_fail(*out_data == NULL, FALSE); @@ -819,27 +771,31 @@ 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) { - gboolean eof; - char c = '\0'; + ssize_t nr; - eof = !_read_fd_buf_c(&read_buf, &c); + nr = read(fd, &c, 1); + if (nr < 0) { + if (errno == EAGAIN) { + struct pollfd pfd; + int r; - if (!eof && c == '\0') { - /* On the first '\0' char, we also assume the data is finished. Abort. */ - read_buf.eof = TRUE; - eof = TRUE; + 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 (!eof && c != '\n') { + if (nr > 0 && 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. */ @@ -893,7 +849,7 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable next: g_string_truncate(line, 0); - if (eof) + if (nr == 0) break; } From cb70a15fd5229234bf689e83d3de5ec0611a9cdf Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 28 Mar 2022 09:40:22 +0200 Subject: [PATCH 2/3] libnm: handle NUL characters in nm_vpn_service_plugin_read_vpn_details() and fix test We expect to read NUL terminated strings. Upon NUL, we should do something. Treat it as a line break. Fixes: 8ae9cf4698b4 ('Revert "libnm: buffer reads in nm_vpn_service_plugin_read_vpn_details()"') (cherry picked from commit 6235815248314c0bd3deb485692881620a859cf9) --- src/libnm-client-impl/nm-vpn-service-plugin.c | 6 ++++++ src/libnm-client-impl/tests/test-libnm.c | 10 ++++++++-- 2 files changed, 14 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 7b04c30799..3493d1db10 100644 --- a/src/libnm-client-impl/nm-vpn-service-plugin.c +++ b/src/libnm-client-impl/nm-vpn-service-plugin.c @@ -795,6 +795,12 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable } break; } + + if (nr > 0 && c == '\0') { + /* '\0' are not supported. Replace with newline. */ + c = '\n'; + } + if (nr > 0 && c != '\n') { g_string_append_c(line, c); if (line->len > 512 * 1024) { diff --git a/src/libnm-client-impl/tests/test-libnm.c b/src/libnm-client-impl/tests/test-libnm.c index 0f600a4a01..58b71e285c 100644 --- a/src/libnm-client-impl/tests/test-libnm.c +++ b/src/libnm-client-impl/tests/test-libnm.c @@ -2546,8 +2546,14 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "DONE\n" "\n" "", - READ_VPN_DETAIL_DATA({"some\nkey-2", "val2"}, {"some-key", "string"}, ), - READ_VPN_DETAIL_DATA(), ); + READ_VPN_DETAIL_DATA({"some\nkey-2", "val2"}, + {"some-key", "string"}, + {"key3\nkey-2", "val3"}, ), + READ_VPN_DETAIL_DATA({"some-secret", "val3"}, + {"key-inval", "in\xc1val"}, + {"ke\xc0yx", "inval"}, + {"keyx", ""}, + {"", "val3"}), ); } /*****************************************************************************/ From 74cbcbbb59c1dde48865366c292e1612615e2382 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 28 Mar 2022 10:37:08 +0200 Subject: [PATCH 3/3] libnm/tests: test that nm_vpn_service_plugin_read_vpn_details() does not consume "QUIT" command (cherry picked from commit b1b1ee8cc41af4f57902560f6769daf15852311e) --- src/libnm-client-impl/tests/test-libnm.c | 95 ++++++++++++++++++++++-- 1 file changed, 88 insertions(+), 7 deletions(-) diff --git a/src/libnm-client-impl/tests/test-libnm.c b/src/libnm-client-impl/tests/test-libnm.c index 58b71e285c..e68c246b3e 100644 --- a/src/libnm-client-impl/tests/test-libnm.c +++ b/src/libnm-client-impl/tests/test-libnm.c @@ -2272,6 +2272,12 @@ _memfd_create(const char *name) return -1; } +typedef enum { + READ_VPN_DETAIL_TYPE_GOOD, + READ_VPN_DETAIL_TYPE_NO_DONE, + READ_VPN_DETAIL_TYPE_BROKEN, +} ReadVpnDetailType; + typedef struct { const char *key; const char *val; @@ -2285,6 +2291,7 @@ _do_read_vpn_details_impl1(const char *file, int memfd, char *mem, gsize len, + ReadVpnDetailType detail_type, const ReadVpnDetailData *expected_data, guint expected_data_len, const ReadVpnDetailData *expected_secrets, @@ -2294,10 +2301,27 @@ _do_read_vpn_details_impl1(const char *file, off_t lseeked; gs_unref_hashtable GHashTable *data = NULL; gs_unref_hashtable GHashTable *secrets = NULL; + char ch; + gboolean append_quit; + char read_buf[1024]; + gssize n_read; + gssize i; written = write(memfd, mem, len); g_assert_cmpint(written, ==, (gssize) len); + append_quit = nmtst_get_rand_bool(); + + if (append_quit) { + if (len > 0 && mem[len - 1] != '\n') { + ch = '\n'; + written = write(memfd, &ch, 1); + g_assert_cmpint(written, ==, 1); + } + written = write(memfd, "QUIT", 4); + g_assert_cmpint(written, ==, 4); + } + lseeked = lseek(memfd, 0, SEEK_SET); g_assert_cmpint(lseeked, ==, 0); @@ -2361,10 +2385,58 @@ _do_read_vpn_details_impl1(const char *file, _assert_hash(secrets, expected_secrets, expected_secrets_len); #undef _assert_hash + + n_read = read(memfd, read_buf, sizeof(read_buf)); + + if (0) { + gs_free char *ss = NULL; + + g_print(">>>> n_read=%zd; \"%s\"", + n_read, + n_read > 0 ? ( + ss = nm_utils_buf_utf8safe_escape_cp(read_buf, + n_read, + NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL)) + : ""); + } + + g_assert_cmpint(n_read, >=, 0); + g_assert_cmpint(n_read, <, sizeof(read_buf)); + + i = read(memfd, &ch, 1); + g_assert_cmpint(i, ==, 0); + + switch (detail_type) { + case READ_VPN_DETAIL_TYPE_GOOD: + g_assert_cmpint(n_read, >=, 0); + i = 0; + while (i < n_read && NM_IN_SET(read_buf[i], '\n')) + i++; + if (append_quit) + g_assert_cmpmem("QUIT", 4, &read_buf[i], n_read - i); + else + g_assert_cmpint(n_read - i, ==, 0); + break; + case READ_VPN_DETAIL_TYPE_NO_DONE: + g_assert_cmpint(n_read, ==, 0); + break; + case READ_VPN_DETAIL_TYPE_BROKEN: + g_assert_cmpint(n_read, >, 0); + if (append_quit) { + g_assert_cmpint(n_read, >, 4); + g_assert(memmem(&read_buf[i], n_read + i, "QUIT", 4)); + } + break; + default: + g_assert_not_reached(); + break; + } + return TRUE; } #define _do_read_vpn_details_impl0(str, \ + detail_type, \ expected_data, \ expected_data_len, \ expected_secrets, \ @@ -2385,6 +2457,7 @@ _do_read_vpn_details_impl1(const char *file, _memfd, \ "" str "", \ NM_STRLEN(str), \ + (detail_type), \ expected_data, \ expected_data_len, \ expected_secrets, \ @@ -2393,14 +2466,16 @@ _do_read_vpn_details_impl1(const char *file, } \ G_STMT_END -#define _do_read_vpn_details_empty(str) _do_read_vpn_details_impl0(str, NULL, 0, NULL, 0, {}) +#define _do_read_vpn_details_empty(str) \ + _do_read_vpn_details_impl0(str, READ_VPN_DETAIL_TYPE_GOOD, NULL, 0, NULL, 0, {}) -#define _do_read_vpn_details(str, expected_data, expected_secrets, pre_setup_cmd) \ - _do_read_vpn_details_impl0(str, \ - expected_data, \ - G_N_ELEMENTS(expected_data), \ - expected_secrets, \ - G_N_ELEMENTS(expected_secrets), \ +#define _do_read_vpn_details(str, detail_type, expected_data, expected_secrets, pre_setup_cmd) \ + _do_read_vpn_details_impl0(str, \ + detail_type, \ + expected_data, \ + G_N_ELEMENTS(expected_data), \ + expected_secrets, \ + G_N_ELEMENTS(expected_secrets), \ pre_setup_cmd) static void @@ -2426,6 +2501,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "DONE\n" "\n" "", + READ_VPN_DETAIL_TYPE_GOOD, READ_VPN_DETAIL_DATA({"some-key", "string"}, {"some-other-key", "val2"}, ), READ_VPN_DETAIL_DATA({"some-secret", "val3"}, ), ); @@ -2433,6 +2509,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "DATA_KEY=some-key\n" "DATA_VAL=string\n" "DONE\n", + READ_VPN_DETAIL_TYPE_GOOD, READ_VPN_DETAIL_DATA({"some-key", "string"}, ), READ_VPN_DETAIL_DATA(), ); @@ -2461,6 +2538,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "\n" "DONE\n" "", + READ_VPN_DETAIL_TYPE_BROKEN, READ_VPN_DETAIL_DATA({"some-key", "string\ncontinued after a line break"}, ), READ_VPN_DETAIL_DATA({"key names\ncan have\ncontinuations too", "value"}, ), ); @@ -2474,6 +2552,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "=continuations too\n" "SECRET_VAL=value\n" "", + READ_VPN_DETAIL_TYPE_NO_DONE, READ_VPN_DETAIL_DATA({"some-key", "string\ncontinued after a line break"}, ), READ_VPN_DETAIL_DATA({"key names\ncan have\ncontinuations too", "value"}, ), ); @@ -2507,6 +2586,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "DONE\n" "\n" "", + READ_VPN_DETAIL_TYPE_GOOD, READ_VPN_DETAIL_DATA({"some\nkey-2", "val2"}, {"some-key", "string"}, {"key3\nkey-2", "val3"}, ), @@ -2546,6 +2626,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "DONE\n" "\n" "", + READ_VPN_DETAIL_TYPE_GOOD, READ_VPN_DETAIL_DATA({"some\nkey-2", "val2"}, {"some-key", "string"}, {"key3\nkey-2", "val3"}, ),