Improve handling of connection URIs when executing remote commands

Previously, if connection URIs were in use and "repmgr standby switchover"
was executed, repmgr would pass the connection URI as-is to the demotion
candidate to execute "repmgr node rejoin". However the presence of
unescaped ampersands in the connection URI was causing the rejoin command
to be incorrectly executed.

Addresses GitHub #525.

HISTORY

@@ -2,6 +2,8 @@
         repmgr: add --version-number command line option (Ian)
         repmgr: add --terse option to "cluster show"; GitHub #521 (Ian)
         repmgr: add --dry-run option to "standby promote"; GitHub #522 (Ian)
+        repmgr: "standby switchover": improve handling of connection URIs when
+          executing "node rejoin" on the demotion candidate; GitHub #525 (Ian)
         repmgrd: check binary and extension major versions match; GitHub #515 (Ian)
 
 4.2.1   2018-??-??

compat.c

@@ -98,9 +98,42 @@ appendShellString(PQExpBuffer buf, const char *str)
 
 		if (*p == '\'')
 			appendPQExpBufferStr(buf, "'\"'\"'");
+		else if (*p == '&')
+			appendPQExpBufferStr(buf, "\\&");
 		else
 			appendPQExpBufferChar(buf, *p);
 	}
 
 	appendPQExpBufferChar(buf, '\'');
 }
+
+/*
+ * Adapted from: src/fe_utils/string_utils.c
+ */
+void
+appendRemoteShellString(PQExpBuffer buf, const char *str)
+{
+	const char *p;
+
+	appendPQExpBufferStr(buf, "\\'");
+
+	for (p = str; *p; p++)
+	{
+		if (*p == '\n' || *p == '\r')
+		{
+			fprintf(stderr,
+					_("shell command argument contains a newline or carriage return: \"%s\"\n"),
+					str);
+			exit(ERR_BAD_CONFIG);
+		}
+
+		if (*p == '\'')
+			appendPQExpBufferStr(buf, "'\"'\"'");
+		else if (*p == '&')
+			appendPQExpBufferStr(buf, "\\&");
+		else
+			appendPQExpBufferChar(buf, *p);
+	}
+
+	appendPQExpBufferStr(buf, "\\'");
+}

compat.h

@@ -27,4 +27,6 @@ extern void appendConnStrVal(PQExpBuffer buf, const char *str);
 
 extern void appendShellString(PQExpBuffer buf, const char *str);
 
+extern void appendRemoteShellString(PQExpBuffer buf, const char *str);
+
 #endif

dbutils.c

@@ -162,7 +162,8 @@ _establish_db_connection(const char *conninfo, const bool exit_on_error, const b
 
 	if (parse_success == false)
 	{
-		log_error(_("unable to pass provided conninfo string:\n	 %s"), errmsg);
+		log_error(_("unable to parse provided conninfo string \"%s\""), conninfo);
+		log_detail("%s", errmsg);
 		free_conninfo_params(&conninfo_params);
 		return NULL;
 	}
@@ -653,7 +654,6 @@ parse_conninfo_string(const char *conninfo_str, t_conninfo_param_list *param_lis
 			if (strcmp(option->keyword, "servicefile") == 0)
 				continue;
 		}
-
 		param_set(param_list, option->keyword, option->val);
 	}
 
@@ -740,6 +740,41 @@ param_list_to_string(t_conninfo_param_list *param_list)
 }
 
 
+/*
+ * Run a conninfo string through the parser, and pass it back as a normal
+ * conninfo string. This is mainly intended for converting connection URIs
+ * to parameter/value conninfo strings.
+ *
+ * Caller must free returned pointer.
+ */
+
+char *
+normalize_conninfo_string(const char *conninfo_str)
+{
+	t_conninfo_param_list conninfo_params = T_CONNINFO_PARAM_LIST_INITIALIZER;
+	bool		parse_success = false;
+	char	   *normalized_string = NULL;
+	char	   *errmsg = NULL;
+
+	initialize_conninfo_params(&conninfo_params, false);
+
+	parse_success = parse_conninfo_string(conninfo_str, &conninfo_params, &errmsg, false);
+
+	if (parse_success == false)
+	{
+		log_error(_("unable to parse provided conninfo string \"%s\""), conninfo_str);
+		log_detail("%s", errmsg);
+		free_conninfo_params(&conninfo_params);
+		return NULL;
+	}
+
+
+	normalized_string = param_list_to_string(&conninfo_params);
+	free_conninfo_params(&conninfo_params);
+
+	return normalized_string;
+}
+
 /*
  * check whether the libpq version in use recognizes the "passfile" parameter
  * (should be 9.6 and later)

dbutils.h

@@ -402,6 +402,7 @@ void		param_set_ine(t_conninfo_param_list *param_list, const char *param, const
 char	   *param_get(t_conninfo_param_list *param_list, const char *param);
 bool		parse_conninfo_string(const char *conninfo_str, t_conninfo_param_list *param_list, char **errmsg, bool ignore_local_params);
 char	   *param_list_to_string(t_conninfo_param_list *param_list);
+char	   *normalize_conninfo_string(const char *conninfo_str);
 bool		has_passfile(void);
 
 

doc/appendix-release-notes.sgml

@@ -54,6 +54,25 @@
       </para>
     </sect2>
 
+    <sect2>
+      <title>Bug fixes</title>
+      <para>
+        <itemizedlist>
+
+          <listitem>
+            <para>
+              &repmgr;: when executing <command><link linkend="repmgr-standby-switchover">repmgr standby switchover</link></command>,
+              prevent escaping issues with connection URIs when executing <command><link linkend="repmgr-node-rejoin">repmgr node rejoin</link></command>
+              on the demotion candidate (GitHub #525).
+            </para>
+          </listitem>
+
+        </itemizedlist>
+      </para>
+    </sect2>
+
+
+
   </sect1>
 
   <sect1 id="release-4.2">

repmgr-action-standby.c

@@ -3704,8 +3704,7 @@ do_standby_switchover(void)
 
 			initPQExpBuffer(&command_output);
 
-			command_success = remote_command(
-											 remote_host,
+			command_success = remote_command(remote_host,
 											 runtime_options.remote_user,
 											 remote_command_str.data,
 											 &command_output);
@@ -4360,10 +4359,27 @@ do_standby_switchover(void)
 	initPQExpBuffer(&remote_command_str);
 	make_remote_repmgr_path(&remote_command_str, &remote_node_record);
 
-	appendPQExpBuffer(&remote_command_str,
-					  "%s-d \\'%s\\' node rejoin",
-					  node_rejoin_options.data,
-					  local_node_record.conninfo);
+	/*
+	 * Here we'll coerce the local node's connection string into
+	 * "param=value" format, in case it's configured in URI format,
+	 * to simplify escaping issues when passing the string to the
+	 * remote node.
+	 */
+	{
+		char	   *conninfo_normalized = normalize_conninfo_string(local_node_record.conninfo);
+
+		appendPQExpBuffer(&remote_command_str,
+						  "%s-d  ",
+						  node_rejoin_options.data);
+
+		appendRemoteShellString(&remote_command_str,
+								conninfo_normalized);
+
+		appendPQExpBufferStr(&remote_command_str,
+							 " node rejoin");
+
+		pfree(conninfo_normalized);
+	}
 
 	termPQExpBuffer(&node_rejoin_options);