From 460afe6d502f2a8f158739c176302fd7fa072913 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:18 +0200 Subject: [PATCH 01/15] cloud-setup: fix allocating buffer for GetConfigMetadataMac in _get_config_metadata_ready_check() It's not a severe issue, because the GetConfigMetadataData struct is larger than GetConfigMetadataMac. Fixes: 69f048bf0ca3 ('cloud-setup: add tool for automatic IP configuration in cloud') --- clients/cloud-setup/nmcs-provider-ec2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clients/cloud-setup/nmcs-provider-ec2.c b/clients/cloud-setup/nmcs-provider-ec2.c index c8db31f97f..3b5f6d36a8 100644 --- a/clients/cloud-setup/nmcs-provider-ec2.c +++ b/clients/cloud-setup/nmcs-provider-ec2.c @@ -487,7 +487,7 @@ _get_config_metadata_ready_check (long response_code, if (!response_parsed) response_parsed = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, g_free); - mac_data = g_malloc (sizeof (GetConfigMetadataData) + 1 + p_start_l); + mac_data = g_malloc (sizeof (GetConfigMetadataMac) + 1 + p_start_l); mac_data->iface_idx = iface_idx_counter++; memcpy (mac_data->path, p_start, p_start_l); mac_data->path[p_start_l] = '\0'; From e3bbd267c3c2c39991a012057da80e52dd43ae16 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:28 +0200 Subject: [PATCH 02/15] cloud-setup: add gtk-doc comment for nm_http_client_get_finish() NMHttpClient guarantees that the returned response is %NUL terminated after the returned length of the buffer. That guarantee is important and should be documented. --- clients/cloud-setup/nm-http-client.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/clients/cloud-setup/nm-http-client.c b/clients/cloud-setup/nm-http-client.c index 946ed8ce93..76db6c6ff7 100644 --- a/clients/cloud-setup/nm-http-client.c +++ b/clients/cloud-setup/nm-http-client.c @@ -351,6 +351,21 @@ nm_http_client_get (NMHttpClient *self, } } +/** + * nm_http_client_get_finish: + * @self: the #NMHttpClient instance + * @result: the #GAsyncResult which to complete. + * @out_response_code: (allow-none) (out): the HTTP response code or -1 on other error. + * @out_response_data: (allow-none) (transfer full): the HTTP response data, if any. + * The GBytes buffer is guaranteed to have a trailing NUL character *after* the + * returned buffer size. That means, you can always trust that the buffer is NUL terminated + * and that there is one additional hidden byte after the data. + * Also, the returned buffer is allocated just for you. While GBytes is immutable, you are + * allowed to modify the buffer as it's not used by anybody else. + * @error: the error + * + * Returns: %TRUE on success. + */ gboolean nm_http_client_get_finish (NMHttpClient *self, GAsyncResult *result, From befd971b45163bebfa0a052fd63b4bd4bb47d6ee Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:29 +0200 Subject: [PATCH 03/15] cloud-setup: assert that NMHttpClient returns NUL terminated buffer The behavior is documented at various places, so this assert is less to actually assert it, but as making this condition obvious to the reader of the code. --- clients/cloud-setup/nmcs-provider-ec2.c | 2 ++ clients/cloud-setup/nmcs-provider-gcp.c | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/clients/cloud-setup/nmcs-provider-ec2.c b/clients/cloud-setup/nmcs-provider-ec2.c index 3b5f6d36a8..a3b4c9a0d3 100644 --- a/clients/cloud-setup/nmcs-provider-ec2.c +++ b/clients/cloud-setup/nmcs-provider-ec2.c @@ -449,6 +449,8 @@ _get_config_metadata_ready_check (long response_code, } r_data = g_bytes_get_data (response_data, &r_len); + /* NMHttpClient guarantees that there is a trailing NUL after the data. */ + nm_assert (r_data[r_len] == 0); while (r_len > 0) { const guint8 *p_eol; diff --git a/clients/cloud-setup/nmcs-provider-gcp.c b/clients/cloud-setup/nmcs-provider-gcp.c index ba4016dd15..676dd1f27b 100644 --- a/clients/cloud-setup/nmcs-provider-gcp.c +++ b/clients/cloud-setup/nmcs-provider-gcp.c @@ -246,10 +246,11 @@ _get_config_ips_list_cb (GObject *source, if (error) goto fips_error; + response_str = g_bytes_get_data (response, &response_len); + /* NMHttpClient guarantees that there is a trailing NUL after the data. */ + nm_assert (response_str[response_len] == 0); uri_arr = g_ptr_array_new_with_free_func (g_free); - response_str = g_bytes_get_data (response, &response_len); - while (nm_utils_parse_next_line (&response_str, &response_len, &line, @@ -403,6 +404,9 @@ _get_net_ifaces_list_cb (GObject *source, } response_str = g_bytes_get_data (response, &response_len); + /* NMHttpClient guarantees that there is a trailing NUL after the data. */ + nm_assert (response_str[response_len] == 0); + ifaces_arr = g_ptr_array_new (); gstr = g_string_new (NULL); From e7357419cd767aff36807ea8cabc0155271ddd88 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:30 +0200 Subject: [PATCH 04/15] shared: add nm_str_buf_finalize_to_gbytes() helper --- shared/nm-glib-aux/nm-str-buf.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/shared/nm-glib-aux/nm-str-buf.h b/shared/nm-glib-aux/nm-str-buf.h index 61246969de..f4a3542c4f 100644 --- a/shared/nm-glib-aux/nm-str-buf.h +++ b/shared/nm-glib-aux/nm-str-buf.h @@ -429,6 +429,23 @@ nm_str_buf_finalize (NMStrBuf *strbuf, return g_steal_pointer (&strbuf->_priv_str); } +static inline GBytes * +nm_str_buf_finalize_to_gbytes (NMStrBuf *strbuf) +{ + char *s; + gsize l; + + /* this always returns a non-NULL, newly allocated GBytes instance. + * The data buffer always has an additional NUL character after + * the data, and the data is allocated with malloc. + * + * That means, the caller who takes ownership of the GBytes can + * safely modify the content of the buffer (including the additional + * NUL sentinel). */ + s = nm_str_buf_finalize (strbuf, &l); + return g_bytes_new_take (s ?: g_new0 (char, 1), l); +} + /** * nm_str_buf_destroy: * @strbuf: an initialized #NMStrBuf From e2bd72235863504e899c173b510739ac24ff19a0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:31 +0200 Subject: [PATCH 05/15] shared: refactor nm_utils_parse_next_line() and add tests - add unit test for nm_utils_parse_next_line() - as line delimiter also accept "\r\n" and "\r" (beside "\n", "\0" and EOF). - fix returning lines with embedded "\0" characters. The line ends on the first "\n" or "\0", whatever comes first. The code before didn't ensure that with: line_end = memchr (line_start, '\n', *inout_len); if (!line_end) line_end = memchr (line_start, '\0', *inout_len); --- shared/nm-glib-aux/nm-shared-utils.c | 64 ++++++----- .../nm-glib-aux/tests/test-shared-general.c | 104 ++++++++++++++++++ 2 files changed, 143 insertions(+), 25 deletions(-) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 1569662f8c..9fc4e510ac 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -1080,39 +1080,53 @@ nm_utils_parse_next_line (const char **inout_ptr, const char **out_line, gsize *out_line_len) { + gboolean eol_is_carriage_return; const char *line_start; - const char *line_end; + gsize line_len; - g_return_val_if_fail (inout_ptr, FALSE); - g_return_val_if_fail (inout_len, FALSE); - g_return_val_if_fail (out_line, FALSE); + nm_assert (inout_ptr); + nm_assert (inout_len); + nm_assert (*inout_len == 0 || *inout_ptr); + nm_assert (out_line); + nm_assert (out_line_len); - if (*inout_len <= 0) - goto error; + if (G_UNLIKELY (*inout_len == 0)) + return FALSE; line_start = *inout_ptr; - line_end = memchr (line_start, '\n', *inout_len); - if (!line_end) - line_end = memchr (line_start, '\0', *inout_len); - if (!line_end) { - line_end = line_start + *inout_len; - NM_SET_OUT (inout_len, 0); - } else - NM_SET_OUT (inout_len, *inout_len - (line_end - line_start) - 1); - NM_SET_OUT (out_line, line_start); - NM_SET_OUT (out_line_len, (gsize) (line_end - line_start)); + eol_is_carriage_return = FALSE; + for (line_len = 0; ; line_len++) { + if (line_len >= *inout_len) { + /* if we consumed the entire line, we place the pointer at + * one character after the end. */ + *inout_ptr = &line_start[line_len]; + *inout_len = 0; + goto done; + } + switch (line_start[line_len]) { + case '\r': + eol_is_carriage_return = TRUE; + /* fall-through*/ + case '\0': + case '\n': + *inout_ptr = &line_start[line_len + 1]; + *inout_len = *inout_len - line_len - 1u; + if ( eol_is_carriage_return + && *inout_len > 0 + && (*inout_ptr)[0] == '\n') { + /* also consume "\r\n" as one. */ + (*inout_len)--; + (*inout_ptr)++; + } + goto done; + } + } - if (*inout_len > 0) - NM_SET_OUT (inout_ptr, line_end + 1); - else - NM_SET_OUT (inout_ptr, NULL); +done: + *out_line = line_start; + *out_line_len = line_len; return TRUE; - -error: - NM_SET_OUT (out_line, NULL); - NM_SET_OUT (out_line_len, 0); - return FALSE; } /*****************************************************************************/ diff --git a/shared/nm-glib-aux/tests/test-shared-general.c b/shared/nm-glib-aux/tests/test-shared-general.c index a435e28195..f389770a7a 100644 --- a/shared/nm-glib-aux/tests/test-shared-general.c +++ b/shared/nm-glib-aux/tests/test-shared-general.c @@ -773,6 +773,109 @@ test_nm_str_buf (void) /*****************************************************************************/ +static void +test_nm_utils_parse_next_line (void) +{ + const char *data; + const char *data0; + gsize data_len; + const char *line_start; + gsize line_len; + int i_run; + gsize j, k; + + data = NULL; + data_len = 0; + g_assert (!nm_utils_parse_next_line (&data, &data_len, &line_start, &line_len)); + + for (i_run = 0; i_run < 1000; i_run++) { + gs_unref_ptrarray GPtrArray *strv = g_ptr_array_new_with_free_func (g_free); + gs_unref_ptrarray GPtrArray *strv2 = g_ptr_array_new_with_free_func (g_free); + gsize strv_len = nmtst_get_rand_word_length (NULL); + nm_auto_str_buf NMStrBuf strbuf = NM_STR_BUF_INIT (0, nmtst_get_rand_bool ()); + + /* create a list of random words. */ + for (j = 0; j < strv_len; j++) { + gsize w_len = nmtst_get_rand_word_length (NULL); + NMStrBuf w_buf = NM_STR_BUF_INIT (nmtst_get_rand_uint32 () % (w_len + 1), nmtst_get_rand_bool ()); + + for (k = 0; k < w_len; k++) + nm_str_buf_append_c (&w_buf, '0' + (k % 10)); + nm_str_buf_maybe_expand (&w_buf, 1, TRUE); + g_ptr_array_add (strv, nm_str_buf_finalize (&w_buf, NULL)); + } + + /* join the list of random words with (random) line delimiters + * ("\0", "\n", "\r" or EOF). */ + for (j = 0; j < strv_len; j++) { + nm_str_buf_append (&strbuf, strv->pdata[j]); +again: + switch (nmtst_get_rand_uint32 () % 5) { + case 0: + nm_str_buf_append_c (&strbuf, '\0'); + break; + case 1: + if ( strbuf.len > 0 + && (nm_str_buf_get_str_unsafe (&strbuf))[strbuf.len - 1] == '\r') { + /* the previous line was empty and terminated by "\r". We + * must not join with "\n". Retry. */ + goto again; + } + nm_str_buf_append_c (&strbuf, '\n'); + break; + case 2: + nm_str_buf_append_c (&strbuf, '\r'); + break; + case 3: + nm_str_buf_append (&strbuf, "\r\n"); + break; + case 4: + /* the last word randomly is delimited or not, but not if the last + * word is "". */ + if (j + 1 < strv_len) { + /* it's not the last word. Retry. */ + goto again; + } + g_assert (j == strv_len - 1); + if (((const char *) strv->pdata[j])[0] == '\0') { + /* if the last word was "", we need a delimiter (to parse it back). + * Retry. */ + goto again; + } + /* The final delimiter gets omitted. It's EOF. */ + break; + } + } + + data0 = nm_str_buf_get_str_unsafe (&strbuf); + if ( !data0 + && nmtst_get_rand_bool ()) { + nm_str_buf_maybe_expand (&strbuf, 1, TRUE); + data0 = nm_str_buf_get_str_unsafe (&strbuf); + g_assert (data0); + } + data_len = strbuf.len; + g_assert ((data_len > 0 && data0) || data_len == 0); + data = data0; + while (nm_utils_parse_next_line (&data, &data_len, &line_start, &line_len)) { + g_assert (line_start); + g_assert (line_start >= data0); + g_assert (line_start < &data0[strbuf.len]); + g_assert (!memchr (line_start, '\0', line_len)); + g_ptr_array_add (strv2, g_strndup (line_start, line_len)); + } + g_assert (data_len == 0); + if (data0) + g_assert (data == &data0[strbuf.len]); + else + g_assert (!data); + + g_assert (_nm_utils_strv_cmp_n ((const char *const*) strv->pdata, strv->len, (const char *const*) strv2->pdata, strv2->len) == 0); + } +} + +/*****************************************************************************/ + NMTST_DEFINE (); int main (int argc, char **argv) @@ -794,6 +897,7 @@ int main (int argc, char **argv) g_test_add_func ("/general/test_string_table_lookup", test_string_table_lookup); g_test_add_func ("/general/test_nm_utils_get_next_realloc_size", test_nm_utils_get_next_realloc_size); g_test_add_func ("/general/test_nm_str_buf", test_nm_str_buf); + g_test_add_func ("/general/test_nm_utils_parse_next_line", test_nm_utils_parse_next_line); return g_test_run (); } From 4f542384c3357f913c7c9b2642cf54065addf181 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:32 +0200 Subject: [PATCH 06/15] cloud-setup: use nm_utils_parse_next_line() in _get_config_metadata_ready_check() nm_utils_parse_next_line() has more flexible handling of line endings (for example, also accpting "\0", "\r", "\r\n"). Use it. --- clients/cloud-setup/nmcs-provider-ec2.c | 43 ++++++++----------------- 1 file changed, 14 insertions(+), 29 deletions(-) diff --git a/clients/cloud-setup/nmcs-provider-ec2.c b/clients/cloud-setup/nmcs-provider-ec2.c index a3b4c9a0d3..1427534b68 100644 --- a/clients/cloud-setup/nmcs-provider-ec2.c +++ b/clients/cloud-setup/nmcs-provider-ec2.c @@ -436,7 +436,9 @@ _get_config_metadata_ready_check (long response_code, GetConfigMetadataData *metadata_data = check_user_data; gs_unref_hashtable GHashTable *response_parsed = NULL; const guint8 *r_data; + const char *cur_line; gsize r_len; + gsize cur_line_len; GHashTableIter h_iter; gboolean has_all; const char *c_hwaddr; @@ -452,47 +454,30 @@ _get_config_metadata_ready_check (long response_code, /* NMHttpClient guarantees that there is a trailing NUL after the data. */ nm_assert (r_data[r_len] == 0); - while (r_len > 0) { - const guint8 *p_eol; - const char *p_start; - gsize p_start_l; - gsize p_start_l_2; - char *hwaddr; + while (nm_utils_parse_next_line ((const char **) &r_data, &r_len, &cur_line, &cur_line_len)) { GetConfigMetadataMac *mac_data; + char *hwaddr; - p_start = (const char *) r_data; - - p_eol = memchr (r_data, '\n', r_len); - if (p_eol) { - p_start_l = (p_eol - r_data); - r_len -= p_start_l + 1; - r_data = &p_eol[1]; - } else { - p_start_l = r_len; - r_data = &r_data[r_len]; - r_len = 0; - } - - if (p_start_l == 0) + if (cur_line_len == 0) continue; - p_start_l_2 = p_start_l; - if (p_start[p_start_l_2 - 1] == '/') { - /* trim the trailing "/". */ - p_start_l_2--; - } + /* Truncate the string. It's safe to do, because we own @response_data an it has an + * extra NUL character after the buffer. */ + ((char *) cur_line)[cur_line_len] = '\0'; - hwaddr = nmcs_utils_hwaddr_normalize (p_start, p_start_l_2); + hwaddr = nmcs_utils_hwaddr_normalize (cur_line, + cur_line[cur_line_len - 1u] == '/' + ? (gssize) (cur_line_len - 1u) + : -1); if (!hwaddr) continue; if (!response_parsed) response_parsed = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, g_free); - mac_data = g_malloc (sizeof (GetConfigMetadataMac) + 1 + p_start_l); + mac_data = g_malloc (sizeof (GetConfigMetadataMac) + 1u + cur_line_len); mac_data->iface_idx = iface_idx_counter++; - memcpy (mac_data->path, p_start, p_start_l); - mac_data->path[p_start_l] = '\0'; + memcpy (mac_data->path, cur_line, cur_line_len + 1u); g_hash_table_insert (response_parsed, hwaddr, mac_data); } From 39733352d629885621a8bb7a315061bbe6477ef4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:33 +0200 Subject: [PATCH 07/15] cloud-setup: use NMStrBuf in nmcs_utils_uri_build_concat_v() --- clients/cloud-setup/nm-cloud-setup-utils.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/clients/cloud-setup/nm-cloud-setup-utils.c b/clients/cloud-setup/nm-cloud-setup-utils.c index a32003cb35..d08eb1144e 100644 --- a/clients/cloud-setup/nm-cloud-setup-utils.c +++ b/clients/cloud-setup/nm-cloud-setup-utils.c @@ -6,6 +6,7 @@ #include "nm-glib-aux/nm-time-utils.h" #include "nm-glib-aux/nm-logging-base.h" +#include "nm-glib-aux/nm-str-buf.h" /*****************************************************************************/ @@ -565,15 +566,13 @@ nmcs_utils_uri_build_concat_v (const char *base, const char **components, gsize n_components) { - GString *uri; + NMStrBuf strbuf = NM_STR_BUF_INIT (NM_UTILS_GET_NEXT_REALLOC_SIZE_104, FALSE); nm_assert (base); nm_assert (base[0]); nm_assert (!NM_STR_HAS_SUFFIX (base, "/")); - uri = g_string_sized_new (100); - - g_string_append (uri, base); + nm_str_buf_append (&strbuf, base); if ( n_components > 0 && components[0] @@ -583,18 +582,18 @@ nmcs_utils_uri_build_concat_v (const char *base, * * We only do that for the first component. */ } else - g_string_append_c (uri, '/'); + nm_str_buf_append_c (&strbuf, '/'); while (n_components > 0) { if (!components[0]) { - /* we allow NULL, to indicate nothing to append*/ + /* we allow NULL, to indicate nothing to append */ } else - g_string_append (uri, components[0]); + nm_str_buf_append (&strbuf, components[0]); components++; n_components--; } - return g_string_free (uri, FALSE); + return nm_str_buf_finalize (&strbuf, NULL); } /*****************************************************************************/ From c9c54709b843d2c195985a9267a6297f02396683 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:33 +0200 Subject: [PATCH 08/15] cloud-setup: use NMStrBuf in NMHttpClient to track response --- clients/cloud-setup/nm-http-client.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/clients/cloud-setup/nm-http-client.c b/clients/cloud-setup/nm-http-client.c index 76db6c6ff7..24d42e3d5f 100644 --- a/clients/cloud-setup/nm-http-client.c +++ b/clients/cloud-setup/nm-http-client.c @@ -7,6 +7,7 @@ #include #include "nm-cloud-setup-utils.h" +#include "nm-glib-aux/nm-str-buf.h" #define NM_CURL_DEBUG 0 @@ -118,7 +119,7 @@ typedef struct { CURLcode ehandle_result; CURL *ehandle; char *url; - GString *recv_data; + NMStrBuf recv_data; struct curl_slist *headers; gssize max_data; gulong cancellable_id; @@ -144,8 +145,7 @@ _ehandle_free (EHandleData *edata) g_object_unref (edata->task); - if (edata->recv_data) - g_string_free (edata->recv_data, TRUE); + nm_str_buf_destroy (&edata->recv_data); if (edata->headers) curl_slist_free_all (edata->headers); g_free (edata->url); @@ -191,12 +191,15 @@ _ehandle_complete (EHandleData *edata, _LOG2E (edata, "failed to get response code from curl easy handle"); _LOG2D (edata, "success getting %"G_GSIZE_FORMAT" bytes (response code %ld)", - edata->recv_data->len, + edata->recv_data.len, response_code); _LOG2T (edata, "received %"G_GSIZE_FORMAT" bytes: [[%s]]", - edata->recv_data->len, - nm_utils_buf_utf8safe_escape (edata->recv_data->str, edata->recv_data->len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL, &str_tmp_1)); + edata->recv_data.len, + nm_utils_buf_utf8safe_escape (nm_str_buf_get_str (&edata->recv_data), + edata->recv_data.len, + NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL, + &str_tmp_1)); _ehandle_free_ehandle (edata); @@ -205,7 +208,7 @@ _ehandle_complete (EHandleData *edata, .response_code = response_code, /* This ensures that response_data is always NUL terminated. This is an important guarantee * that NMHttpClient makes. */ - .response_data = g_string_free_to_bytes (g_steal_pointer (&edata->recv_data)), + .response_data = nm_str_buf_finalize_to_gbytes (&edata->recv_data), }; g_task_return_pointer (edata->task, get_result, _get_result_free); @@ -225,14 +228,14 @@ _get_writefunction_cb (char *ptr, size_t size, size_t nmemb, void *user_data) nmemb *= size; if (edata->max_data >= 0) { - nm_assert (edata->recv_data->len <= edata->max_data); - nconsume = (((gsize) edata->max_data) - edata->recv_data->len); + nm_assert (edata->recv_data.len <= edata->max_data); + nconsume = (((gsize) edata->max_data) - edata->recv_data.len); if (nconsume > nmemb) nconsume = nmemb; } else nconsume = nmemb; - g_string_append_len (edata->recv_data, ptr, nconsume); + nm_str_buf_append_len (&edata->recv_data, ptr, nconsume); return nconsume; } @@ -283,7 +286,7 @@ nm_http_client_get (NMHttpClient *self, edata = g_slice_new (EHandleData); *edata = (EHandleData) { .task = nm_g_task_new (self, cancellable, nm_http_client_get, callback, user_data), - .recv_data = g_string_sized_new (NM_MIN (max_data, 245)), + .recv_data = NM_STR_BUF_INIT (0, FALSE), .max_data = max_data, .url = g_strdup (url), .headers = NULL, From 62aec7acd3601f1a433181fdc80bbe0167c5539e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:34 +0200 Subject: [PATCH 09/15] cloud-setup: don't use a GString in _get_config_ips_list_cb() nm_utils_parse_next_line() operates on the response buffer obtained from NMHttpClient. We own this buffer, and we also can rely on the fact that the buffer has a trailing NUL byte after the data. There is no need to clone the string to a GString, just use it directly. --- clients/cloud-setup/nmcs-provider-gcp.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/clients/cloud-setup/nmcs-provider-gcp.c b/clients/cloud-setup/nmcs-provider-gcp.c index 676dd1f27b..13f317e12e 100644 --- a/clients/cloud-setup/nmcs-provider-gcp.c +++ b/clients/cloud-setup/nmcs-provider-gcp.c @@ -255,21 +255,20 @@ _get_config_ips_list_cb (GObject *source, &response_len, &line, &line_len)) { - nm_auto_free_gstring GString *gstr = NULL; gint64 fip_index; - gstr = g_string_new_len (line, line_len); - fip_index = _nm_utils_ascii_str_to_int64 (gstr->str, 10, 0, G_MAXINT64, -1); + /* Truncate the string. It's safe to do, because we own @response_data an it has an + * extra NUL character after the buffer. */ + ((char *) line)[line_len] = '\0'; - if (fip_index < 0) { + fip_index = _nm_utils_ascii_str_to_int64 (line, 10, 0, G_MAXINT64, -1); + if (fip_index < 0) continue; - } - g_string_printf (gstr, - "%"G_GSSIZE_FORMAT"/forwarded-ips/%"G_GINT64_FORMAT, - iface_data->iface_idx, - fip_index); - g_ptr_array_add (uri_arr, g_string_free (g_steal_pointer (&gstr), FALSE)); + g_ptr_array_add (uri_arr, + g_strdup_printf ("%"G_GSSIZE_FORMAT"/forwarded-ips/%"G_GINT64_FORMAT, + iface_data->iface_idx, + fip_index)); } iface_data->n_fips_pending = uri_arr->len; From 3d61b2894111a07ce958f6a554691b199ad990b8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:35 +0200 Subject: [PATCH 10/15] cloud-setup: don't use a GString in loop in _get_net_ifaces_list_cb() nm_utils_parse_next_line() operates on the response buffer obtained from NMHttpClient. We own this buffer, and we also can rely on the fact that the buffer has a trailing NUL byte after the data. There is no need to copy the string to a GString, just use it directly. --- clients/cloud-setup/nmcs-provider-gcp.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/clients/cloud-setup/nmcs-provider-gcp.c b/clients/cloud-setup/nmcs-provider-gcp.c index 13f317e12e..069abfcc7a 100644 --- a/clients/cloud-setup/nmcs-provider-gcp.c +++ b/clients/cloud-setup/nmcs-provider-gcp.c @@ -384,8 +384,6 @@ _get_net_ifaces_list_cb (GObject *source, gs_free_error GError *error = NULL; GCPData *gcp_data = user_data; const char *response_str; - const char *token_start; - const char *token_end; gsize response_len; const char *line; gsize line_len; @@ -416,16 +414,16 @@ _get_net_ifaces_list_cb (GObject *source, GCPIfaceData *iface_data; gssize iface_idx; - token_start = line; - token_end = memchr (token_start, '/', line_len); - - if (!token_end) + if (line_len == 0) continue; - g_string_truncate (gstr, 0); - g_string_append_len (gstr, token_start, token_end - token_start); - iface_idx = _nm_utils_ascii_str_to_int64 (gstr->str, 10, 0, G_MAXSSIZE, -1); + /* Truncate the string. It's safe to do, because we own @response_data an it has an + * extra NUL character after the buffer. */ + ((char *) line)[line_len] = '\0'; + if (line[line_len - 1] == '/') + ((char *) line)[--line_len] = '\0'; + iface_idx = _nm_utils_ascii_str_to_int64 (line, 10, 0, G_MAXSSIZE, -1); if (iface_idx < 0) continue; @@ -487,7 +485,6 @@ get_config (NMCSProvider *provider, .n_ifaces_pending = 0, .error = NULL, .success = FALSE, - }; nm_http_client_poll_get (nmcs_provider_get_http_client (provider), From 2fbc8717ca343fd6bafbe36a10d04db293fc4d88 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:36 +0200 Subject: [PATCH 11/15] cloud-setup: use stack allocated buffer for temporary strings in "nmcs-provider-gcp.c" The maximum length of these strings is known and small. Use a buffer on the stack for them. --- clients/cloud-setup/nmcs-provider-gcp.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/clients/cloud-setup/nmcs-provider-gcp.c b/clients/cloud-setup/nmcs-provider-gcp.c index 069abfcc7a..a879a4eb3a 100644 --- a/clients/cloud-setup/nmcs-provider-gcp.c +++ b/clients/cloud-setup/nmcs-provider-gcp.c @@ -321,7 +321,7 @@ _get_config_iface_cb (GObject *source, gs_free_error GError *error = NULL; gs_free const char *hwaddr = NULL; gs_free const char *uri = NULL; - gs_free char *str = NULL; + char sbuf[100]; GCPData *gcp_data; gcp_data = iface_data->gcp_data; @@ -350,11 +350,10 @@ _get_config_iface_cb (GObject *source, iface_data->iface_idx, hwaddr); - str = g_strdup_printf ("%"G_GSSIZE_FORMAT"/forwarded-ips/", - iface_data->iface_idx); + nm_sprintf_buf (sbuf, "%"G_GSSIZE_FORMAT"/forwarded-ips/", iface_data->iface_idx); nm_http_client_poll_get (NM_HTTP_CLIENT (source), - (uri = _gcp_uri_interfaces (str)), + (uri = _gcp_uri_interfaces (sbuf)), HTTP_TIMEOUT_MS, HTTP_REQ_MAX_DATA, HTTP_POLL_TIMEOUT_MS, @@ -379,7 +378,6 @@ _get_net_ifaces_list_cb (GObject *source, gpointer user_data) { gs_unref_ptrarray GPtrArray *ifaces_arr = NULL; - nm_auto_free_gstring GString *gstr = NULL; gs_unref_bytes GBytes *response = NULL; gs_free_error GError *error = NULL; GCPData *gcp_data = user_data; @@ -405,7 +403,6 @@ _get_net_ifaces_list_cb (GObject *source, nm_assert (response_str[response_len] == 0); ifaces_arr = g_ptr_array_new (); - gstr = g_string_new (NULL); while (nm_utils_parse_next_line (&response_str, &response_len, @@ -443,14 +440,15 @@ _get_net_ifaces_list_cb (GObject *source, for (i = 0; i < ifaces_arr->len; ++i) { GCPIfaceData *data = ifaces_arr->pdata[i]; gs_free const char *uri = NULL; + char sbuf[100]; _LOGD ("GCP interface[%"G_GSSIZE_FORMAT"]: retrieving configuration", data->iface_idx); - g_string_printf (gstr, "%"G_GSSIZE_FORMAT"/mac", data->iface_idx); + nm_sprintf_buf (sbuf, "%"G_GSSIZE_FORMAT"/mac", data->iface_idx); nm_http_client_poll_get (NM_HTTP_CLIENT (source), - (uri = _gcp_uri_interfaces (gstr->str)), + (uri = _gcp_uri_interfaces (sbuf)), HTTP_TIMEOUT_MS, HTTP_REQ_MAX_DATA, HTTP_POLL_TIMEOUT_MS, From ceb75f8ab4f28b076ddcdf34ace4f106bf88d914 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:38 +0200 Subject: [PATCH 12/15] cloud-setup: remove debugging message from _poll_cancelled_cb() --- clients/cloud-setup/nm-cloud-setup-utils.c | 1 - 1 file changed, 1 deletion(-) diff --git a/clients/cloud-setup/nm-cloud-setup-utils.c b/clients/cloud-setup/nm-cloud-setup-utils.c index d08eb1144e..9db3b742c9 100644 --- a/clients/cloud-setup/nm-cloud-setup-utils.c +++ b/clients/cloud-setup/nm-cloud-setup-utils.c @@ -357,7 +357,6 @@ _poll_cancelled_cb (GObject *object, gpointer user_data) PollTaskData *poll_task_data = user_data; GError *error = NULL; - _LOGD (">> poll cancelled"); nm_clear_g_signal_handler (g_task_get_cancellable (poll_task_data->task), &poll_task_data->cancellable_id); nm_utils_error_set_cancelled (&error, FALSE, NULL); From eb2dfa9b4117f34205e8153efcfc19925bba7b4d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:39 +0200 Subject: [PATCH 13/15] cloud-setup: always report success or an GError from nm_http_client_poll_get_finish()/nmcs_utils_poll_finish() Since commit 3bd30f6064c3 ('nmcs: add error message when a HTTP request times out'), the case where polling returns %FALSE without an error is no longer possible. This is preferable, because it follows a consistent API where a function clearly fails or succeeds. So, checking for the error code and the returned boolean is redundant and unnecessary. --- clients/cloud-setup/nm-cloud-setup-utils.c | 16 ++++++++-------- clients/cloud-setup/nm-http-client.c | 14 ++++++++------ 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/clients/cloud-setup/nm-cloud-setup-utils.c b/clients/cloud-setup/nm-cloud-setup-utils.c index 9db3b742c9..39f3b100a1 100644 --- a/clients/cloud-setup/nm-cloud-setup-utils.c +++ b/clients/cloud-setup/nm-cloud-setup-utils.c @@ -258,7 +258,6 @@ _poll_task_data_free (gpointer data) static void _poll_return (PollTaskData *poll_task_data, - gboolean success, GError *error_take) { nm_clear_g_source_inst (&poll_task_data->source_next_poll); @@ -271,7 +270,7 @@ _poll_return (PollTaskData *poll_task_data, if (error_take) g_task_return_error (poll_task_data->task, g_steal_pointer (&error_take)); else - g_task_return_boolean (poll_task_data->task, success); + g_task_return_boolean (poll_task_data->task, TRUE); g_object_unref (poll_task_data->task); } @@ -302,7 +301,7 @@ _poll_done_cb (GObject *source, if ( error || is_finished) { - _poll_return (poll_task_data, TRUE, g_steal_pointer (&error)); + _poll_return (poll_task_data, g_steal_pointer (&error)); return; } @@ -346,8 +345,8 @@ _poll_timeout_cb (gpointer user_data) { PollTaskData *poll_task_data = user_data; - _poll_return (poll_task_data, FALSE, nm_utils_error_new (NM_UTILS_ERROR_UNKNOWN, - "timeout expired")); + _poll_return (poll_task_data, nm_utils_error_new (NM_UTILS_ERROR_UNKNOWN, + "timeout expired")); return G_SOURCE_CONTINUE; } @@ -360,13 +359,13 @@ _poll_cancelled_cb (GObject *object, gpointer user_data) nm_clear_g_signal_handler (g_task_get_cancellable (poll_task_data->task), &poll_task_data->cancellable_id); nm_utils_error_set_cancelled (&error, FALSE, NULL); - _poll_return (poll_task_data, FALSE, error); + _poll_return (poll_task_data, error); } /** * nmcs_utils_poll: * @poll_timeout_ms: if >= 0, then this is the overall timeout for how long we poll. - * When this timeout expires, the request completes with failure (but no error set). + * When this timeout expires, the request completes with failure (and error set). * @ratelimit_timeout_ms: if > 0, we ratelimit the starts from one prope_start_fcn * call to the next. * @sleep_timeout_ms: if > 0, then we wait after a probe finished this timeout @@ -459,7 +458,8 @@ nmcs_utils_poll (int poll_timeout_ms, * %FALSE will be returned. * If the probe returned a failure, this returns %FALSE and the error * provided by @probe_finish_fcn. - * If the request times out, this returns %FALSE without error set. + * If the request times out, this returns %FALSE with error set. + * Error is always set if (and only if) the function returns %FALSE. */ gboolean nmcs_utils_poll_finish (GAsyncResult *result, diff --git a/clients/cloud-setup/nm-http-client.c b/clients/cloud-setup/nm-http-client.c index 24d42e3d5f..7f042c8af6 100644 --- a/clients/cloud-setup/nm-http-client.c +++ b/clients/cloud-setup/nm-http-client.c @@ -505,10 +505,12 @@ _poll_get_done_cb (GObject *source, success = nmcs_utils_poll_finish (result, NULL, &error); + nm_assert ((!!success) == (!error)); + if (error) g_task_return_error (poll_get_data->task, g_steal_pointer (&error)); else - g_task_return_boolean (poll_get_data->task, success); + g_task_return_boolean (poll_get_data->task, TRUE); g_object_unref (poll_get_data->task); } @@ -587,10 +589,11 @@ nm_http_client_poll_get_finish (NMHttpClient *self, task = G_TASK (result); success = g_task_propagate_boolean (task, &local_error); - if ( local_error - || !success) { - if (local_error) - g_propagate_error (error, g_steal_pointer (&local_error)); + + nm_assert ((!!success) == (!local_error)); + + if (local_error) { + g_propagate_error (error, g_steal_pointer (&local_error)); NM_SET_OUT (out_response_code, -1); NM_SET_OUT (out_response_data, NULL); return FALSE; @@ -600,7 +603,6 @@ nm_http_client_poll_get_finish (NMHttpClient *self, NM_SET_OUT (out_response_code, poll_get_data->response_code); NM_SET_OUT (out_response_data, g_steal_pointer (&poll_get_data->response_data)); - return TRUE; } From 53bdd81800fab05a6474d370d52105a1740b1905 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:40 +0200 Subject: [PATCH 14/15] cloud-setup: ensure that nm_http_client_get_finish() always returns success or error --- clients/cloud-setup/nm-http-client.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/clients/cloud-setup/nm-http-client.c b/clients/cloud-setup/nm-http-client.c index 7f042c8af6..294259f261 100644 --- a/clients/cloud-setup/nm-http-client.c +++ b/clients/cloud-setup/nm-http-client.c @@ -367,7 +367,7 @@ nm_http_client_get (NMHttpClient *self, * allowed to modify the buffer as it's not used by anybody else. * @error: the error * - * Returns: %TRUE on success. + * Returns: %TRUE on success or %FALSE with an error code. */ gboolean nm_http_client_get_finish (NMHttpClient *self, @@ -382,6 +382,9 @@ nm_http_client_get_finish (NMHttpClient *self, g_return_val_if_fail (nm_g_task_is_valid (result, self, nm_http_client_get), FALSE); get_result = g_task_propagate_pointer (G_TASK (result), error); + + nm_assert ((!!get_result) == (!error)); + if (!get_result) { NM_SET_OUT (out_response_code, -1); NM_SET_OUT (out_response_data, NULL); @@ -394,7 +397,6 @@ nm_http_client_get_finish (NMHttpClient *self, NM_SET_OUT (out_response_data, g_steal_pointer (&get_result->response_data)); _get_result_free (get_result); - return TRUE; } @@ -465,11 +467,14 @@ _poll_get_probe_finish_fcn (GObject *source, &response_data, &local_error); - if (!success) { + nm_assert ((!!success) == (!local_error)); + + if (local_error) { if (nm_utils_error_is_cancelled (local_error)) { g_propagate_error (error, g_steal_pointer (&local_error)); return TRUE; } + /* any other error. Continue polling. */ return FALSE; } @@ -486,8 +491,10 @@ _poll_get_probe_finish_fcn (GObject *source, return TRUE; } - if (!success) + if (!success) { + /* Not yet ready. Continue polling. */ return FALSE; + } poll_get_data->response_code = response_code; poll_get_data->response_data = g_steal_pointer (&response_data); From 9702f79db6ffce2a3319d0a710fe9498380d17e0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Jun 2020 09:52:41 +0200 Subject: [PATCH 15/15] cloud-setup: don't check redundant error results from nm_http_client_poll_get_finish() nm_http_client_poll_get_finish() can only either succeed (returning TRUE and setting no GError), or failing (returning FALSE and setting GError). Checking for both is redundant and unnecessary. --- clients/cloud-setup/nmcs-provider-ec2.c | 42 +++++++++---------------- clients/cloud-setup/nmcs-provider-gcp.c | 19 +++-------- 2 files changed, 20 insertions(+), 41 deletions(-) diff --git a/clients/cloud-setup/nmcs-provider-ec2.c b/clients/cloud-setup/nmcs-provider-ec2.c index 1427534b68..1578a33bed 100644 --- a/clients/cloud-setup/nmcs-provider-ec2.c +++ b/clients/cloud-setup/nmcs-provider-ec2.c @@ -90,13 +90,12 @@ _detect_get_meta_data_done_cb (GObject *source, gs_unref_object GTask *task = user_data; gs_free_error GError *get_error = NULL; gs_free_error GError *error = NULL; - gboolean success; - success = nm_http_client_poll_get_finish (NM_HTTP_CLIENT (source), - result, - NULL, - NULL, - &get_error); + nm_http_client_poll_get_finish (NM_HTTP_CLIENT (source), + result, + NULL, + NULL, + &get_error); if (nm_utils_error_is_cancelled (get_error)) { g_task_return_error (task, g_steal_pointer (&get_error)); @@ -112,14 +111,6 @@ _detect_get_meta_data_done_cb (GObject *source, return; } - if (!success) { - nm_utils_error_set (&error, - NM_UTILS_ERROR_UNKNOWN, - "failure to detect EC2 metadata"); - g_task_return_error (task, g_steal_pointer (&error)); - return; - } - g_task_return_boolean (task, TRUE); } @@ -191,31 +182,28 @@ _get_config_fetch_done_cb (NMHttpClient *http_client, gboolean is_local_ipv4) { GetConfigIfaceData *iface_data; - NMCSProviderGetConfigTaskData *get_config_data; const char *hwaddr = NULL; gs_unref_bytes GBytes *response_data = NULL; gs_free_error GError *error = NULL; - gboolean success; - NMCSProviderGetConfigIfaceData *config_iface_data; nm_utils_user_data_unpack (user_data, &iface_data, &hwaddr); - success = nm_http_client_poll_get_finish (http_client, - result, - NULL, - &response_data, - &error); + nm_http_client_poll_get_finish (http_client, + result, + NULL, + &response_data, + &error); + if (nm_utils_error_is_cancelled (error)) return; - get_config_data = iface_data->get_config_data; - - config_iface_data = g_hash_table_lookup (get_config_data->result_dict, hwaddr); - - if (success) { + if (!error) { + NMCSProviderGetConfigIfaceData *config_iface_data; in_addr_t tmp_addr; int tmp_prefix; + config_iface_data = g_hash_table_lookup (iface_data->get_config_data->result_dict, hwaddr); + if (is_local_ipv4) { gs_free const char **s_addrs = NULL; gsize i, len; diff --git a/clients/cloud-setup/nmcs-provider-gcp.c b/clients/cloud-setup/nmcs-provider-gcp.c index a879a4eb3a..7a8f3ef893 100644 --- a/clients/cloud-setup/nmcs-provider-gcp.c +++ b/clients/cloud-setup/nmcs-provider-gcp.c @@ -46,13 +46,12 @@ _detect_get_meta_data_done_cb (GObject *source, gs_unref_object GTask *task = user_data; gs_free_error GError *get_error = NULL; gs_free_error GError *error = NULL; - gboolean success; - success = nm_http_client_poll_get_finish (NM_HTTP_CLIENT (source), - result, - NULL, - NULL, - &get_error); + nm_http_client_poll_get_finish (NM_HTTP_CLIENT (source), + result, + NULL, + NULL, + &get_error); if (nm_utils_error_is_cancelled (get_error)) { g_task_return_error (task, g_steal_pointer (&get_error)); @@ -68,14 +67,6 @@ _detect_get_meta_data_done_cb (GObject *source, return; } - if (!success) { - nm_utils_error_set (&error, - NM_UTILS_ERROR_UNKNOWN, - "failure to detect GCP metadata"); - g_task_return_error (task, g_steal_pointer (&error)); - return; - } - g_task_return_boolean (task, TRUE); }