cli/polkit: make parsing polkit-agent-helper-1 protocol more conformingth/cli-command-cleanup

- in io_watch_have_data(), ensure that we handle incomplete lines
that don't yet have a newline by waiting for more data. That means,
if the current content of the in_buffer does not have a newline, we
wait longer.

- in io_watch_have_data(), implement (and ignore) certain commands
instead of failing the request.

- in io_watch_have_data(), no longer g_compress() the entire line.
"polkitagenthelper-pam.c" never backslash escapes the command, it
only escapes the arguments. Of course, there should be no difference
in practice, except that we don't want to handle escape sequences
in the commands.

- in io_watch_have_data(), compare SUCCESS/FAILURE literally.
"polkitagenthelper-pam.c" never appends any trailing garbage to these
commands, and we shouldn't handle that (although "polkitagentsession.c"
does).

- when io_watch_have_data() completes with success, we cannot destroy
AuthRequest right away. It probably still has data pending that we first
need to write to the polkit helper. Wait longer, and let io_watch_can_write()
complete the request.

- ensure we always answer the GDBusMethodInvocation. Otherwise, it gets
leaked.

- use NMStrBuf instead of GString.

3 files changed, 162 insertions, 140 deletions

diff --git a/clients/common/nm-polkit-listener.c b/clients/common/nm-polkit-listener.c
index ae40206ca7..7691ff4e06 100644
--- a/clients/common/nm-polkit-listener.c
+++ b/clients/common/nm-polkit-listener.c
@@ -27,6 +27,7 @@
 #include <fcntl.h>
 
 #include "nm-glib-aux/nm-dbus-aux.h"
+#include "nm-glib-aux/nm-str-buf.h"
 #include "nm-glib-aux/nm-secret-utils.h"
 #include "nm-glib-aux/nm-io-utils.h"
 #include "nm-libnm-core-intern/nm-auth-subject.h"
@@ -84,19 +85,22 @@ typedef struct {
 	CList request_lst;
 
 	NMPolkitListener *listener;
+	GSource *child_stdout_watch_source;
+	GSource *child_stdin_watch_source;
+	GDBusMethodInvocation *dbus_invocation;
 	char *action_id;
 	char *message;
 	char *username;
 	char *cookie;
-	GString *in_buffer;
-	GString *out_buffer;
-	size_t out_buffer_offset;
+	NMStrBuf in_buffer;
+	NMStrBuf out_buffer;
+	gsize out_buffer_offset;
 
 	int child_stdout;
 	int child_stdin;
-	GSource *child_stdout_watch_source;
-	GSource *child_stdin_watch_source;
-	GDBusMethodInvocation *dbus_invocation;
+
+	bool request_any_response:1;
+	bool request_is_completed:1;
 } AuthRequest;
 
 static const GDBusInterfaceInfo interface_info = NM_DEFINE_GDBUS_INTERFACE_INFO_INIT (
@@ -123,10 +127,18 @@ static const GDBusInterfaceInfo interface_info = NM_DEFINE_GDBUS_INTERFACE_INFO_
 );
 
 static void
-remove_request (AuthRequest *request)
+auth_request_complete (AuthRequest *request, gboolean success)
 {
 	c_list_unlink (&request->request_lst);
 
+	if (success)
+		g_dbus_method_invocation_return_value (request->dbus_invocation, NULL);
+	else {
+		g_dbus_method_invocation_return_dbus_error (request->dbus_invocation,
+		                                            POLKIT_DBUS_ERROR_FAILED,
+		                                            "");
+	}
+
 	nm_clear_g_free (&request->action_id);
 	nm_clear_g_free (&request->message);
 	nm_clear_g_free (&request->username);
@@ -134,10 +146,8 @@ remove_request (AuthRequest *request)
 	nm_clear_g_source_inst (&request->child_stdout_watch_source);
 	nm_clear_g_source_inst (&request->child_stdin_watch_source);
 
-	nm_explicit_bzero (request->out_buffer->str,
-	                   request->out_buffer->len);
-	g_string_free (request->out_buffer, TRUE);
-	g_string_free (request->in_buffer, TRUE);
+	nm_str_buf_destroy (&request->in_buffer);
+	nm_str_buf_destroy (&request->out_buffer);
 
 	if (request->child_stdout != -1) {
 		nm_close (request->child_stdout);
@@ -149,7 +159,7 @@ remove_request (AuthRequest *request)
 		request->child_stdin = -1;
 	}
 
-	g_slice_free (AuthRequest, request);
+	nm_g_slice_free (request);
 }
 
 static gboolean
@@ -384,60 +394,42 @@ retrieve_session_id (NMPolkitListener *self)
 	                        self);
 }
 
-static void
-complete_authentication (AuthRequest *request,
-                         gboolean result)
-{
-	if (result) {
-		g_dbus_method_invocation_return_value (request->dbus_invocation, NULL);
-	} else {
-		g_dbus_method_invocation_return_dbus_error (request->dbus_invocation,
-		                                            "org.freedesktop.PolicyKit1.Error.Failed",
-		                                            "");
-	}
-	remove_request (request);
-}
-
 static gboolean
 io_watch_can_write (int fd,
                     GIOCondition condition,
                     gpointer user_data)
 {
 	AuthRequest *request = user_data;
-	ssize_t n_written;
-	gboolean done = FALSE;
+	gssize n_written;
 
-	if (condition & G_IO_HUP ||
-	    condition & G_IO_ERR) {
-		done = TRUE;
+	if (NM_FLAGS_ANY (condition, (G_IO_HUP | G_IO_ERR)))
 		goto done;
-	}
 
 	n_written = write (request->child_stdin,
-	                   &request->out_buffer->str[request->out_buffer_offset],
-	                   request->out_buffer->len - request->out_buffer_offset);
+	                   &((nm_str_buf_get_str_unsafe (&request->out_buffer))[request->out_buffer_offset]),
+	                   request->out_buffer.len - request->out_buffer_offset);
 
-	if (n_written < 0 && errno != EAGAIN) {
-		done = TRUE;
+	if (   n_written < 0
+	    && errno != EAGAIN)
 		goto done;
-	}
 
 	if (n_written > 0) {
-		if ((size_t) n_written == (request->out_buffer->len - request->out_buffer_offset)) {
-			done = TRUE;
+		if ((gsize) n_written >= (request->out_buffer.len - request->out_buffer_offset)) {
+			nm_assert ((gsize) n_written == (request->out_buffer.len - request->out_buffer_offset));
 			goto done;
 		}
-		request->out_buffer_offset += n_written;
+		request->out_buffer_offset += (gsize) n_written;
 	}
 
+	return G_SOURCE_CONTINUE;
+
 done:
-	if (done) {
-		nm_explicit_bzero (request->out_buffer->str,
-		                   request->out_buffer->len);
-		g_string_set_size (request->out_buffer, 0);
-		request->out_buffer_offset = 0;
-		nm_clear_g_source_inst (&request->child_stdin_watch_source);
-	}
+	nm_str_buf_set_size (&request->out_buffer, 0, TRUE, FALSE);
+	request->out_buffer_offset = 0;
+	nm_clear_g_source_inst (&request->child_stdin_watch_source);
+
+	if (request->request_is_completed)
+		auth_request_complete (request, TRUE);
 
 	return G_SOURCE_CONTINUE;
 }
@@ -447,11 +439,11 @@ queue_string_to_helper (AuthRequest *request, const char *response)
 {
 	g_return_if_fail (response);
 
-	g_string_append (request->out_buffer, response);
+	if (!nm_str_buf_is_initalized (&request->out_buffer))
+		nm_str_buf_init (&request->out_buffer, strlen (response) + 2u,  TRUE);
 
-	if (   request->out_buffer->len == 0
-	    || request->out_buffer->str[request->out_buffer->len - 1] != '\n')
-		g_string_append_c (request->out_buffer, '\n');
+	nm_str_buf_append (&request->out_buffer, response);
+	nm_str_buf_ensure_trailing_c (&request->out_buffer, '\n');
 
 	if (!request->child_stdin_watch_source) {
 		request->child_stdin_watch_source = nm_g_unix_fd_source_new (request->child_stdin,
@@ -471,74 +463,87 @@ io_watch_have_data (int fd,
                     gpointer user_data)
 {
 	AuthRequest *request = user_data;
-	gs_free char *unescaped = NULL;
-	char *response = NULL;
-	char* line_terminator = 0;
-	gboolean auth_result = FALSE;
-	gboolean complete_auth = FALSE;
-	ssize_t n_read;
-
-	if (condition & G_IO_HUP ||
-	    condition & G_IO_ERR) {
-		complete_auth = TRUE;
-		auth_result = FALSE;
-		goto out;
-	}
-
-	n_read = nm_utils_fd_read (fd, request->in_buffer);
-
-	if (n_read == -EAGAIN) {
-		return G_SOURCE_CONTINUE;
-	}
+	gboolean auth_result;
+	gssize n_read;
+
+	if (NM_FLAGS_ANY (condition, G_IO_HUP | G_IO_ERR))
+		n_read = -EIO;
+	else
+		n_read = nm_utils_fd_read (fd, &request->in_buffer);
+
+	if (n_read <= 0) {
+		if (n_read == -EAGAIN) {
+			/* wait longer. */
+			return G_SOURCE_CONTINUE;
+		}
 
-	if (n_read < 0) {
-		complete_auth = TRUE;
-		auth_result = FALSE;
+		/* Either an error or EOF happened. The data we parsed so far was not relevant.
+		 * Regardless of what we still have unprocessed in the receive buffers, we are done.
+		 *
+		 * We would expect that the other side completed with SUCCESS or FAILURE. Apparently
+		 * it didn't. If we had any good request, we assume success. */
+		auth_result = request->request_any_response;
 		goto out;
 	}
 
-	line_terminator = strchr (request->in_buffer->str, '\n');
-	if (!line_terminator) {
-		return G_SOURCE_CONTINUE;
-	}
-	*line_terminator = '\0';
-
-	unescaped = g_strcompress (request->in_buffer->str);
+	while (TRUE) {
+		char *line_terminator;
+		const char *line;
 
-	if (NM_STR_HAS_PREFIX (unescaped, "PAM_PROMPT_ECHO")) {
-		/* FIXME(cli-async): emit signal and wait for response (blocking) */
-		g_signal_emit (request->listener,
-		               signals[REQUEST_SYNC],
-		               0,
-		               request->action_id,
-		               request->message,
-		               request->username,
-		               &response);
-
-		if (response) {
-			queue_string_to_helper (request, response);
-			nm_free_secret (response);
+		line = nm_str_buf_get_str (&request->in_buffer);
+		line_terminator = (char *) strchr (line, '\n');
+		if (!line_terminator) {
+			/* We don't have a complete line. Wait longer. */
+			return G_SOURCE_CONTINUE;
+		}
+		line_terminator[0] = '\0';
+
+		if (   NM_STR_HAS_PREFIX (line, "PAM_PROMPT_ECHO_OFF ")
+		    || NM_STR_HAS_PREFIX (line, "PAM_PROMPT_ECHO_ON ")) {
+			nm_auto_free_secret char *response = NULL;
+
+			/* FIXME(cli-async): emit signal and wait for response (blocking) */
+			g_signal_emit (request->listener,
+			               signals[REQUEST_SYNC],
+			               0,
+			               request->action_id,
+			               request->message,
+			               request->username,
+			               &response);
+
+			if (response) {
+				queue_string_to_helper (request, response);
+				request->request_any_response = TRUE;
+				goto erase_line;
+			}
+			auth_result = FALSE;
+		} else if (nm_streq (line, "SUCCESS"))
+			auth_result = TRUE;
+		else if (nm_streq (line, "FAILURE"))
+			auth_result = FALSE;
+		else if (   NM_STR_HAS_PREFIX (line, "PAM_ERROR_MSG ")
+		         || NM_STR_HAS_PREFIX (line, "PAM_TEXT_INFO ")) {
+			/* ignore. */
+			goto erase_line;
 		} else {
-			complete_auth = TRUE;
+			/* unknown command. Fail. */
 			auth_result = FALSE;
 		}
-	} else if (NM_STR_HAS_PREFIX (unescaped, "SUCCESS")) {
-		complete_auth = TRUE;
-		auth_result = TRUE;
-	} else if (NM_STR_HAS_PREFIX (unescaped, "FAILURE")) {
-		complete_auth = TRUE;
-		auth_result = FALSE;
-	} else {
-		complete_auth = TRUE;
-		auth_result = FALSE;
+
+		break;
+erase_line:
+		nm_str_buf_erase (&request->in_buffer, 0, line_terminator - line + 1u, TRUE);
 	}
 
 out:
-	g_string_set_size (request->in_buffer, 0);
+	request->request_is_completed = TRUE;
+	nm_clear_g_source_inst (&request->child_stdout_watch_source);
+	if (   auth_result
+	    && request->child_stdin_watch_source) {
+		/* we need to wait for the buffer to send the response. */
+	} else
+		auth_request_complete (request, auth_result);
 
-	if (complete_auth) {
-		complete_authentication (request, auth_result);
-	}
 	return G_SOURCE_CONTINUE;
 }
 
@@ -553,7 +558,7 @@ begin_authentication (AuthRequest *request)
 	};
 
 	if (!request->username) {
-		complete_authentication (request, FALSE);
+		auth_request_complete (request, FALSE);
 		return;
 	}
 
@@ -576,7 +581,7 @@ begin_authentication (AuthRequest *request)
 		               0,
 		               "The PolicyKit setuid helper 'polkit-agent-helper-1' has not been found");
 
-		complete_authentication (request, FALSE);
+		auth_request_complete (request, FALSE);
 		return;
 	}
 
@@ -623,19 +628,21 @@ create_request (NMPolkitListener *listener,
                 char *username_take,
                 const char *cookie)
 {
-	AuthRequest *request = g_slice_new0(AuthRequest);
+	AuthRequest *request;
 
-	request->listener = listener;
-	request->dbus_invocation = invocation;
-	request->action_id = g_strdup (action_id);
-	request->message = g_strdup (message);
-	request->username = g_steal_pointer (&username_take);
-	request->cookie = g_strdup (cookie);
-	request->in_buffer = g_string_new ("");
+	request = g_slice_new (AuthRequest);
+	*request = (AuthRequest) {
+		.listener             = listener,
+		.dbus_invocation      = invocation,
+		.action_id            = g_strdup (action_id),
+		.message              = g_strdup (message),
+		.username             = g_steal_pointer (&username_take),
+		.cookie               = g_strdup (cookie),
+		.request_any_response = FALSE,
+		.request_is_completed = FALSE,
+	};
 
-	/* preallocate a large enough buffer so that
-	 * secrets don't get reallocated, thus leaked */
-	request->out_buffer = g_string_sized_new (1024);
+	nm_str_buf_init (&request->in_buffer,  NM_UTILS_GET_NEXT_REALLOC_SIZE_1000, FALSE);
 
 	c_list_link_tail (&listener->request_lst_head, &request->request_lst);
 	return request;
@@ -696,7 +703,7 @@ dbus_method_call_cb (GDBusConnection *connection,
 		}
 
 		/* Complete a cancelled request with success. */
-		complete_authentication (request, TRUE);
+		auth_request_complete (request, TRUE);
 		return;
 	}
 
@@ -895,9 +902,8 @@ dispose (GObject *object)
 
 	nm_clear_g_cancellable (&self->cancellable);
 
-	while ((request = c_list_first_entry (&self->request_lst_head, AuthRequest, request_lst))) {
-		remove_request (request);
-	}
+	while ((request = c_list_first_entry (&self->request_lst_head, AuthRequest, request_lst)))
+		auth_request_complete (request, FALSE);
 
 	if (self->dbus_connection) {
 		nm_clear_g_dbus_connection_signal (self->dbus_connection,
diff --git a/shared/nm-glib-aux/nm-io-utils.c b/shared/nm-glib-aux/nm-io-utils.c
index 9620c688df..776c63e111 100644
--- a/shared/nm-glib-aux/nm-io-utils.c
+++ b/shared/nm-glib-aux/nm-io-utils.c
@@ -11,6 +11,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 
+#include "nm-str-buf.h"
 #include "nm-shared-utils.h"
 #include "nm-secret-utils.h"
 #include "nm-errno.h"
@@ -418,27 +419,40 @@ nm_utils_file_stat (const char *filename, struct stat *out_st)
  * @fd: the fd to read from.
  * @out_string: (out): output string where read bytes will be stored.
  *
- * Returns:  <0 on failure, which is -(errno)
- *           0 on EOF or if the call would block (if the fd is nonblocking),
- *           >0 on success, which is the number of bytes read  */
-ssize_t
-nm_utils_fd_read (int fd, GString *out_string)
+ * Returns: <0 on failure, which is -(errno).
+ *          0 on EOF.
+ *          >0 on success, which is the number of bytes read.  */
+gssize
+nm_utils_fd_read (int fd, NMStrBuf *out_string)
 {
-	size_t start_len;
-	ssize_t n_read;
+	gsize buf_available;
+	gssize n_read;
+	int errsv;
 
 	g_return_val_if_fail (fd >= 0, -1);
 	g_return_val_if_fail (out_string, -1);
 
-	start_len = out_string->len;
-	g_string_set_size (out_string, start_len + 1024);
-
-	n_read = read (fd, &out_string->str[start_len], 1024);
-	if (n_read < 0)
-		return -NM_ERRNO_NATIVE (errno);
+	/* If the buffer size is 0, we allocate NM_UTILS_GET_NEXT_REALLOC_SIZE_1000 (1000 bytes)
+	 * the first time. Afterwards, the buffer grows exponentially.
+	 *
+	 * Note that with @buf_available, we always would read as much buffer as we actually
+	 * have reserved. */
+	nm_str_buf_maybe_expand (out_string, NM_UTILS_GET_NEXT_REALLOC_SIZE_1000, FALSE);
+
+	buf_available = out_string->allocated - out_string->len;
+
+	n_read = read (fd,
+	               &((nm_str_buf_get_str_unsafe (out_string))[out_string->len]),
+	               buf_available);
+	if (n_read < 0) {
+		errsv = errno;
+		return -NM_ERRNO_NATIVE (errsv);
+	}
 
-	if (n_read > 0)
-		g_string_set_size (out_string, start_len + (gsize) n_read);
+	if (n_read > 0) {
+		nm_assert ((gsize) n_read <= buf_available);
+		nm_str_buf_set_size (out_string, out_string->len + (gsize) n_read, TRUE, FALSE);
+	}
 
 	return n_read;
 }
diff --git a/shared/nm-glib-aux/nm-io-utils.h b/shared/nm-glib-aux/nm-io-utils.h
index dd2f499d6f..31326fc79a 100644
--- a/shared/nm-glib-aux/nm-io-utils.h
+++ b/shared/nm-glib-aux/nm-io-utils.h
@@ -47,7 +47,9 @@ gboolean nm_utils_file_set_contents (const char *filename,
                                      int *out_errsv,
                                      GError **error);
 
-ssize_t nm_utils_fd_read (int fd, GString *out_string);
+struct _NMStrBuf;
+
+gssize nm_utils_fd_read (int fd, struct _NMStrBuf *out_string);
 
 struct stat;