Use strtol() in place of atoi() to better verify integer parameters

Per GitHub #127

config.c

@@ -198,8 +198,10 @@ parse_config(t_configuration_options *options)
 		if (strcmp(name, "cluster") == 0)
 			strncpy(options->cluster_name, value, MAXLEN);
 		else if (strcmp(name, "node") == 0)
+			// VV
 			options->node = atoi(value);
 		else if (strcmp(name, "upstream_node") == 0)
+			// VV
 			options->upstream_node = atoi(value);
 		else if (strcmp(name, "conninfo") == 0)
 			strncpy(options->conninfo, value, MAXLEN);
@@ -232,6 +234,7 @@ parse_config(t_configuration_options *options)
 			}
 		}
 		else if (strcmp(name, "priority") == 0)
+			// VV
 			options->priority = atoi(value);
 		else if (strcmp(name, "node_name") == 0)
 			strncpy(options->node_name, value, MAXLEN);
@@ -240,15 +243,19 @@ parse_config(t_configuration_options *options)
 		else if (strcmp(name, "follow_command") == 0)
 			strncpy(options->follow_command, value, MAXLEN);
 		else if (strcmp(name, "master_response_timeout") == 0)
+			// VV
 			options->master_response_timeout = atoi(value);
 		/* 'primary_response_timeout' as synonym for 'master_response_timeout' -
 		 * we'll switch terminology in a future release
 		 */
 		else if (strcmp(name, "primary_response_timeout") == 0)
+			// VV
 			options->master_response_timeout = atoi(value);
 		else if (strcmp(name, "reconnect_attempts") == 0)
+			// VV
 			options->reconnect_attempts = atoi(value);
 		else if (strcmp(name, "reconnect_interval") == 0)
+			// VV
 			options->reconnect_intvl = atoi(value);
 		else if (strcmp(name, "pg_bindir") == 0)
 			strncpy(options->pg_bindir, value, MAXLEN);
@@ -259,10 +266,13 @@ parse_config(t_configuration_options *options)
 		else if (strcmp(name, "logfile") == 0)
 			strncpy(options->logfile, value, MAXLEN);
 		else if (strcmp(name, "monitor_interval_secs") == 0)
+			// VV
 			options->monitor_interval_secs = atoi(value);
 		else if (strcmp(name, "retry_promote_interval_secs") == 0)
+			// VV
 			options->retry_promote_interval_secs = atoi(value);
 		else if (strcmp(name, "use_replication_slots") == 0)
+			// VV
 			options->use_replication_slots = atoi(value);
 		else if (strcmp(name, "event_notification_command") == 0)
 			strncpy(options->event_notification_command, value, MAXLEN);
@@ -305,7 +315,7 @@ parse_config(t_configuration_options *options)
 		exit(ERR_BAD_CONFIG);
 	}
 
-	if (options->node == 0)
+	if (options->node <= 0)
 	{
 		log_err(_("'node' must be an integer greater than zero\n"));
 		exit(ERR_BAD_CONFIG);
@@ -674,6 +684,66 @@ reload_config(t_configuration_options *orig_options)
 }
 
 
+/*
+ * Convert provided string to an integer using strtol;
+ * on error exit
+ */
+int
+repmgr_atoi(const char *value, const char *config_item, void (*error_callback)(char *error_message))
+{
+	char	  *endptr;
+	long	   longval = 0;
+	char	   error_message_buf[MAXLEN] = "";
+
+	/* It's possible that some versions of strtol() don't treat an empty
+	 * string as an error.
+	 */
+
+	if (*value == '\0')
+	{
+		snprintf(error_message_buf,
+				 MAXLEN,
+				 _("No value provided for \"%s\""),
+				 config_item);
+	}
+	else
+	{
+		errno = 0;
+		longval = strtol(value, &endptr, 10);
+
+		if (value == endptr || errno)
+		{
+			snprintf(error_message_buf,
+					 MAXLEN,
+					 _("Invalid value provided for \"%s\": %s"),
+					 config_item, value);
+		}
+	}
+
+	/* Currently there are no values which could be negative */
+	if (longval < 0)
+	{
+		snprintf(error_message_buf,
+				 MAXLEN,
+				 _("\"%s\" cannot be a negative value (provided: %s)"),
+				 config_item, value);
+	}
+
+	/* Error message buffer is set */
+	if (error_message_buf[0] != '\0')
+	{
+		if (error_callback == NULL)
+		{
+			log_err("%s\n", error_message_buf);
+			exit(ERR_BAD_CONFIG);
+		}
+
+		error_callback(error_message_buf);
+	}
+
+	return (int32) longval;
+}
+
 
 /*
  * Split argument into old_dir and new_dir and append to tablespace mapping

config.h

@@ -88,5 +88,8 @@ bool		reload_config(t_configuration_options *orig_options);
 bool		parse_config(t_configuration_options *options);
 void		parse_line(char *buff, char *name, char *value);
 char	   *trim(char *s);
+int			repmgr_atoi(const char *s,
+						const char *config_item,
+						void (*error_callback)(char *error_message));
 
 #endif

repmgr.c

@@ -176,6 +176,12 @@ main(int argc, char **argv)
 	while ((c = getopt_long(argc, argv, "?Vd:h:p:U:S:D:l:f:R:w:k:FWIvb:r:c", long_options,
 							&optindex)) != -1)
 	{
+		/*
+		 * NOTE: some integer parameters (e.g. -p/--port) are stored internally
+		 * as strings. We use repmgr_atoi() to check these but discard the
+		 * returned integer; the callback passed to repmgr_atoi() will append the
+		 * error message to the list.
+		 */
 		switch (c)
 		{
 			case '?':
@@ -191,8 +197,10 @@ main(int argc, char **argv)
 				strncpy(runtime_options.host, optarg, MAXLEN);
 				break;
 			case 'p':
-				if (atoi(optarg) > 0)
+				repmgr_atoi(optarg, "-p/--port", error_list_append);
-					strncpy(runtime_options.masterport, optarg, MAXLEN);
+				strncpy(runtime_options.masterport,
+						optarg,
+						MAXLEN);
 				break;
 			case 'U':
 				strncpy(runtime_options.username, optarg, MAXLEN);
@@ -204,8 +212,11 @@ main(int argc, char **argv)
 				strncpy(runtime_options.dest_dir, optarg, MAXFILENAME);
 				break;
 			case 'l':
-				if (atoi(optarg) > 0)
+				/* -l/--local-port is deprecated */
-					strncpy(runtime_options.localport, optarg, MAXLEN);
+				repmgr_atoi(optarg, "-l/--local-port", error_list_append);
+				strncpy(runtime_options.localport,
+						optarg,
+						MAXLEN);
 				break;
 			case 'f':
 				strncpy(runtime_options.config_file, optarg, MAXLEN);
@@ -214,17 +225,14 @@ main(int argc, char **argv)
 				strncpy(runtime_options.remote_user, optarg, MAXLEN);
 				break;
 			case 'w':
-				if (atoi(optarg) > 0)
+				repmgr_atoi(optarg, "-w/--wal-keep-segments", error_list_append);
-				{
+				strncpy(runtime_options.wal_keep_segments,
-					strncpy(runtime_options.wal_keep_segments, optarg, MAXLEN);
+						optarg,
-					wal_keep_segments_used = true;
+						MAXLEN);
-				}
+				wal_keep_segments_used = true;
 				break;
 			case 'k':
-				if (atoi(optarg) > 0)
+				runtime_options.keep_history = repmgr_atoi(optarg, "-k/--keep-history", error_list_append);
-					runtime_options.keep_history = atoi(optarg);
-				else
-					runtime_options.keep_history = 0;
 				break;
 			case 'F':
 				runtime_options.force = true;
@@ -288,6 +296,7 @@ main(int argc, char **argv)
 		}
 	}
 
+
 	/* Exit here already if errors in command line options found */
 	if (cli_errors.head != NULL)
 	{
@@ -683,7 +692,7 @@ do_cluster_cleanup(void)
 	entries_to_delete = atoi(PQgetvalue(res, 0, 0));
 	PQclear(res);
 
-	if(entries_to_delete == 0)
+	if (entries_to_delete == 0)
 	{
 		log_info(_("cluster cleanup: no monitoring records to delete\n"));
 		PQfinish(master_conn);
@@ -1495,7 +1504,7 @@ do_standby_clone(void)
 		}
 
 
-		if(server_version_num >= 90500 && tablespace_map_rewrite == true)
+		if (server_version_num >= 90500 && tablespace_map_rewrite == true)
 		{
 			PQExpBufferData tablespace_map_filename;
 			FILE	   *tablespace_map_file;
@@ -3713,7 +3722,8 @@ error_list_append(char *error_message)
 		exit(ERR_BAD_CONFIG);
 	}
 
-	cell->error_message = error_message;
+	cell->error_message = pg_malloc0(MAXLEN);
+	strncpy(cell->error_message, error_message, MAXLEN);
 
 	if (cli_errors.tail)
 	{