From 1953ec74597fbd41b8df20bebd865caeab2c3cb0 Mon Sep 17 00:00:00 2001 From: Ian Barwick Date: Thu, 28 Mar 2019 10:37:57 +0900 Subject: [PATCH] Restrict "node_name" to maximum 63 characters In "recovery.conf", the configuration parameter "node_name" is used as the "application_name" value, which will be truncated by PostgreSQL to 63 characters (NAMEDATALEN - 1). repmgr sometimes needs to be able to extract the application name from pg_stat_replication to determine if a node is connected (e.g. when executing "repmgr standby register"), so the comparison will fail if "node_name" exceeds 63 characters. --- configfile.c | 20 +++++++++++++------ configfile.h | 2 +- dbutils.c | 8 ++++---- dbutils.h | 4 ++-- doc/configuration-file-required-settings.sgml | 3 +++ repmgr-action-cluster.c | 6 ++++-- repmgr-action-cluster.h | 4 ++-- repmgr-action-standby.c | 2 +- repmgr-client-global.h | 2 +- repmgr-client.c | 12 ++++++++--- repmgr.conf.sample | 1 + 11 files changed, 42 insertions(+), 22 deletions(-) diff --git a/configfile.c b/configfile.c index 5995bc84..3234f873 100644 --- a/configfile.c +++ b/configfile.c @@ -484,7 +484,14 @@ _parse_config(t_configuration_options *options, ItemList *error_list, ItemList * node_id_found = true; } else if (strcmp(name, "node_name") == 0) - strncpy(options->node_name, value, MAXLEN); + { + if (strlen(value) < sizeof(options->node_name)) + strncpy(options->node_name, value, sizeof(options->node_name)); + else + item_list_append_format(error_list, + _("value for \"node_name\" must contain fewer than %lu characters"), + sizeof(options->node_name)); + } else if (strcmp(name, "conninfo") == 0) strncpy(options->conninfo, value, MAXLEN); else if (strcmp(name, "data_directory") == 0) @@ -494,11 +501,12 @@ _parse_config(t_configuration_options *options, ItemList *error_list, ItemList * else if (strcmp(name, "replication_user") == 0) { - if (strlen(value) < NAMEDATALEN) - strncpy(options->replication_user, value, NAMEDATALEN); + if (strlen(value) < sizeof(options->replication_user)) + strncpy(options->replication_user, value, sizeof(options->replication_user)); else - item_list_append(error_list, - _("value for \"replication_user\" must contain fewer than " STR(NAMEDATALEN) " characters")); + item_list_append_format(error_list, + _("value for \"replication_user\" must contain fewer than %lu characters"), + sizeof(options->replication_user)); } else if (strcmp(name, "pg_bindir") == 0) strncpy(options->pg_bindir, value, MAXPGPATH); @@ -1196,7 +1204,7 @@ reload_config(t_configuration_options *orig_options, t_server_type server_type) return false; } - if (strncmp(new_options.node_name, orig_options->node_name, MAXLEN) != 0) + if (strncmp(new_options.node_name, orig_options->node_name, sizeof(orig_options->node_name)) != 0) { log_warning(_("\"node_name\" cannot be changed, keeping current configuration")); return false; diff --git a/configfile.h b/configfile.h index ab82174d..b0c54413 100644 --- a/configfile.h +++ b/configfile.h @@ -76,7 +76,7 @@ typedef struct { /* node information */ int node_id; - char node_name[MAXLEN]; + char node_name[NAMEDATALEN]; char conninfo[MAXLEN]; char replication_user[NAMEDATALEN]; char data_directory[MAXPGPATH]; diff --git a/dbutils.c b/dbutils.c index b47dbaa0..ed453baf 100644 --- a/dbutils.c +++ b/dbutils.c @@ -2201,9 +2201,9 @@ _populate_node_record(PGresult *res, t_node_info *node_info, int row, bool init_ node_info->upstream_node_id = atoi(PQgetvalue(res, row, 2)); } - strncpy(node_info->node_name, PQgetvalue(res, row, 3), MAXLEN); + strncpy(node_info->node_name, PQgetvalue(res, row, 3), sizeof(node_info->node_name)); strncpy(node_info->conninfo, PQgetvalue(res, row, 4), MAXLEN); - strncpy(node_info->repluser, PQgetvalue(res, row, 5), NAMEDATALEN); + strncpy(node_info->repluser, PQgetvalue(res, row, 5), sizeof(node_info->repluser)); strncpy(node_info->slot_name, PQgetvalue(res, row, 6), MAXLEN); strncpy(node_info->location, PQgetvalue(res, row, 7), MAXLEN); node_info->priority = atoi(PQgetvalue(res, row, 8)); @@ -2211,7 +2211,7 @@ _populate_node_record(PGresult *res, t_node_info *node_info, int row, bool init_ strncpy(node_info->config_file, PQgetvalue(res, row, 10), MAXPGPATH); /* This won't normally be set */ - strncpy(node_info->upstream_node_name, PQgetvalue(res, row, 11), MAXLEN); + strncpy(node_info->upstream_node_name, PQgetvalue(res, row, 11), sizeof(node_info->upstream_node_name)); /* Set remaining struct fields with default values */ @@ -5771,7 +5771,7 @@ get_bdr_other_node_name(PGconn *conn, int node_id, char *node_name) if (PQresultStatus(res) == PGRES_TUPLES_OK) { - strncpy(node_name, PQgetvalue(res, 0, 0), MAXLEN); + strncpy(node_name, PQgetvalue(res, 0, 0), NAMEDATALEN); } else { diff --git a/dbutils.h b/dbutils.h index fceb14c8..d579ca9e 100644 --- a/dbutils.h +++ b/dbutils.h @@ -134,8 +134,8 @@ typedef struct s_node_info int node_id; int upstream_node_id; t_server_type type; - char node_name[MAXLEN]; - char upstream_node_name[MAXLEN]; + char node_name[NAMEDATALEN]; + char upstream_node_name[NAMEDATALEN]; char conninfo[MAXLEN]; char repluser[NAMEDATALEN]; char location[MAXLEN]; diff --git a/doc/configuration-file-required-settings.sgml b/doc/configuration-file-required-settings.sgml index 5a72447a..feff833c 100644 --- a/doc/configuration-file-required-settings.sgml +++ b/doc/configuration-file-required-settings.sgml @@ -39,6 +39,9 @@ called standby1 (for example), things will be confusing to say the least. + + Maximum length is 63 characters. + diff --git a/repmgr-action-cluster.c b/repmgr-action-cluster.c index cbe02f74..28265737 100644 --- a/repmgr-action-cluster.c +++ b/repmgr-action-cluster.c @@ -1063,7 +1063,9 @@ build_cluster_matrix(t_node_matrix_rec ***matrix_rec_dest, int *name_length, Ite matrix_rec_list[i] = (t_node_matrix_rec *) pg_malloc0(sizeof(t_node_matrix_rec)); matrix_rec_list[i]->node_id = cell->node_info->node_id; - strncpy(matrix_rec_list[i]->node_name, cell->node_info->node_name, MAXLEN); + strncpy(matrix_rec_list[i]->node_name, + cell->node_info->node_name, + sizeof(cell->node_info->node_name)); /* * Find the maximum length of a node name @@ -1278,7 +1280,7 @@ build_cluster_crosscheck(t_node_status_cube ***dest_cube, int *name_length, Item cube[h] = (t_node_status_cube *) pg_malloc(sizeof(t_node_status_cube)); cube[h]->node_id = cell->node_info->node_id; - strncpy(cube[h]->node_name, cell->node_info->node_name, MAXLEN); + strncpy(cube[h]->node_name, cell->node_info->node_name, sizeof(cell->node_info->node_name)); /* * Find the maximum length of a node name diff --git a/repmgr-action-cluster.h b/repmgr-action-cluster.h index ed89b2bb..f8f1c08f 100644 --- a/repmgr-action-cluster.h +++ b/repmgr-action-cluster.h @@ -30,14 +30,14 @@ typedef struct typedef struct { int node_id; - char node_name[MAXLEN]; + char node_name[NAMEDATALEN]; t_node_status_rec **node_status_list; } t_node_matrix_rec; typedef struct { int node_id; - char node_name[MAXLEN]; + char node_name[NAMEDATALEN]; t_node_matrix_rec **matrix_list_rec; } t_node_status_cube; diff --git a/repmgr-action-standby.c b/repmgr-action-standby.c index a4dd063a..2145b6aa 100644 --- a/repmgr-action-standby.c +++ b/repmgr-action-standby.c @@ -213,7 +213,7 @@ do_standby_clone(void) param_set(&recovery_conninfo, "application_name", config_file_options.node_name); get_conninfo_value(config_file_options.conninfo, "application_name", application_name); - if (strlen(application_name) && strncmp(application_name, config_file_options.node_name, MAXLEN) != 0) + if (strlen(application_name) && strncmp(application_name, config_file_options.node_name, sizeof(config_file_options.node_name)) != 0) { log_notice(_("\"application_name\" is set in repmgr.conf but will be replaced by the node name")); } diff --git a/repmgr-client-global.h b/repmgr-client-global.h index 46dfb66e..fe56cef9 100644 --- a/repmgr-client-global.h +++ b/repmgr-client-global.h @@ -70,7 +70,7 @@ typedef struct /* general node options */ int node_id; - char node_name[MAXLEN]; + char node_name[NAMEDATALEN]; char data_dir[MAXPGPATH]; int remote_node_id; diff --git a/repmgr-client.c b/repmgr-client.c index f0991799..c828b658 100644 --- a/repmgr-client.c +++ b/repmgr-client.c @@ -356,9 +356,15 @@ main(int argc, char **argv) /* --node-name */ case OPT_NODE_NAME: - strncpy(runtime_options.node_name, optarg, MAXLEN); + { + if (strlen(optarg) < sizeof(runtime_options.node_name)) + strncpy(runtime_options.node_name, optarg, sizeof(runtime_options.node_name)); + else + item_list_append_format(&cli_errors, + _("value for \"--node-name\" must contain fewer than %lu characters"), + sizeof(runtime_options.node_name)); break; - + } /* --remote-node-id */ case OPT_REMOTE_NODE_ID: runtime_options.remote_node_id = repmgr_atoi(optarg, "--remote-node-id", &cli_errors, MIN_NODE_ID); @@ -3001,7 +3007,7 @@ init_node_record(t_node_info *node_record) strncpy(node_record->location, "default", MAXLEN); - strncpy(node_record->node_name, config_file_options.node_name, MAXLEN); + strncpy(node_record->node_name, config_file_options.node_name, sizeof(node_record->node_name)); strncpy(node_record->conninfo, config_file_options.conninfo, MAXLEN); strncpy(node_record->config_file, config_file_path, MAXPGPATH); diff --git a/repmgr.conf.sample b/repmgr.conf.sample index 2750534b..faf929f1 100644 --- a/repmgr.conf.sample +++ b/repmgr.conf.sample @@ -28,6 +28,7 @@ # node's current role, e.g. 'primary' or 'standby1', # as roles can change and it will be confusing if # the current primary is called 'standby1'. + # Maximum length is 63 characters. #conninfo='' # Database connection information as a conninfo string. # All servers in the cluster must be able to connect to