From e12be52fa844c55f680f3f7d72f660f28d96e028 Mon Sep 17 00:00:00 2001 From: Ian Barwick Date: Fri, 6 Nov 2015 09:27:21 +0900 Subject: [PATCH] Use strtol() in place of atoi() to better verify integer parameters Per GitHub #127 --- config.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- config.h | 3 +++ repmgr.c | 42 ++++++++++++++++++++------------- 3 files changed, 100 insertions(+), 17 deletions(-) diff --git a/config.c b/config.c index 5f25487c..f7df910d 100644 --- a/config.c +++ b/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 diff --git a/config.h b/config.h index 5731d140..3d2a2637 100644 --- a/config.h +++ b/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 diff --git a/repmgr.c b/repmgr.c index c91c7b6c..0711a290 100644 --- a/repmgr.c +++ b/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) - strncpy(runtime_options.masterport, optarg, MAXLEN); + repmgr_atoi(optarg, "-p/--port", error_list_append); + 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) - strncpy(runtime_options.localport, optarg, MAXLEN); + /* -l/--local-port is deprecated */ + 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) - { - strncpy(runtime_options.wal_keep_segments, optarg, MAXLEN); - wal_keep_segments_used = true; - } + repmgr_atoi(optarg, "-w/--wal-keep-segments", error_list_append); + strncpy(runtime_options.wal_keep_segments, + optarg, + MAXLEN); + wal_keep_segments_used = true; break; case 'k': - if (atoi(optarg) > 0) - runtime_options.keep_history = atoi(optarg); - else - runtime_options.keep_history = 0; + runtime_options.keep_history = repmgr_atoi(optarg, "-k/--keep-history", error_list_append); 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) {