From 91dcfd8b33c0dba3e2eb83f899e57102c29e3ee1 Mon Sep 17 00:00:00 2001 From: Eric Bollengier Date: Tue, 14 Nov 2017 15:54:00 +0100 Subject: [PATCH] Fix race condition between setip and the access to CLIENT::address() Fix concurrency issue between setAddress() and address() we need to copy the address in an argument, the variable can be freed at any time. --- bacula/src/dird/backup.c | 3 ++- bacula/src/dird/dird_conf.c | 33 +++++++++++++++++++-------------- bacula/src/dird/dird_conf.h | 2 +- bacula/src/dird/fd_cmds.c | 10 ++++++++-- bacula/src/dird/snapshot.c | 10 +++++++--- bacula/src/dird/ua_cmds.c | 19 ++++++++++++------- bacula/src/dird/ua_dotcmds.c | 13 ++++++++----- bacula/src/dird/ua_status.c | 11 +++++++---- bacula/src/lib/util.c | 4 ++-- 9 files changed, 66 insertions(+), 39 deletions(-) diff --git a/bacula/src/dird/backup.c b/bacula/src/dird/backup.c index 10f301f011..47888eecc3 100644 --- a/bacula/src/dird/backup.c +++ b/bacula/src/dird/backup.c @@ -356,6 +356,7 @@ bool send_client_addr_to_sd(JCR *jcr) { int tls_need = BNET_TLS_NONE; BSOCK *sd = jcr->store_bsock; + POOL_MEM buf; /* TLS Requirement for the client */ if (jcr->client->tls_enable) { @@ -368,7 +369,7 @@ bool send_client_addr_to_sd(JCR *jcr) /* * Send Client address to the SD */ - sd->fsend(clientaddr, jcr->client->address(), jcr->client->FDport, tls_need); + sd->fsend(clientaddr, jcr->client->address(buf.addr()), jcr->client->FDport, tls_need); if (!response(jcr, sd, OKclient, "Client", DISPLAY_ERROR)) { return false; } diff --git a/bacula/src/dird/dird_conf.c b/bacula/src/dird/dird_conf.c index 155126860c..02fae0baba 100644 --- a/bacula/src/dird/dird_conf.c +++ b/bacula/src/dird/dird_conf.c @@ -123,19 +123,17 @@ void CLIENT::setNumConcurrentJobs(int32_t num) num, globals->name); } -char *CLIENT::address() +char *CLIENT::address(POOLMEM *&buf) { - if (!globals) { - return client_address; - } - if (!globals->SetIPaddress) { - return client_address; + P(globals_mutex); + if (!globals || !globals->SetIPaddress) { + pm_strcpy(buf, client_address); + + } else { + pm_strcpy(buf, globals->SetIPaddress); } - /* TODO: Fix concurrency issue between setAddress() and address() - * we need to copy the address in an argument, the variable can be - * freed at any time. - */ - return globals->SetIPaddress; + V(globals_mutex); + return buf; } void CLIENT::setAddress(char *addr) @@ -833,6 +831,7 @@ void dump_resource(int type, RES *ares, void sendit(void *sock, const char *fmt, char ed1[100], ed2[100], ed3[100]; DEVICE *dev; UAContext *ua = (UAContext *)sock; + POOLMEM *buf; if (res == NULL) { sendit(sock, _("No %s resource defined\n"), res_to_str(type)); @@ -881,10 +880,12 @@ void dump_resource(int type, RES *ares, void sendit(void *sock, const char *fmt, if (!acl_access_ok(ua, Client_ACL, res->res_client.name())) { break; } + buf = get_pool_memory(PM_FNAME); sendit(sock, _("Client: Name=%s Enabled=%d Address=%s FDport=%d MaxJobs=%u NumJobs=%u\n"), res->res_client.name(), res->res_client.is_enabled(), - res->res_client.address(), res->res_client.FDport, + res->res_client.address(buf), res->res_client.FDport, res->res_client.MaxConcurrentJobs, res->res_client.getNumConcurrentJobs()); + free_pool_memory(buf); sendit(sock, _(" JobRetention=%s FileRetention=%s AutoPrune=%d\n"), edit_utime(res->res_client.JobRetention, ed1, sizeof(ed1)), edit_utime(res->res_client.FileRetention, ed2, sizeof(ed2)), @@ -2510,6 +2511,7 @@ extern "C" char *job_code_callback_director(JCR *jcr, const char* param, char *b if (jcr == NULL) { return nothing; } + ASSERTD(buflen < 255, "buflen must be long enough to hold an ip address"); switch (param[0]) { case 'f': if (jcr->fileset) { @@ -2517,8 +2519,11 @@ extern "C" char *job_code_callback_director(JCR *jcr, const char* param, char *b } break; case 'h': - if (jcr->client && jcr->client->address()) { - return jcr->client->address(); + if (jcr->client) { + POOL_MEM tmp; + jcr->client->address(tmp.addr()); + bstrncpy(buf, tmp.c_str(), buflen); + return buf; } break; case 'p': diff --git a/bacula/src/dird/dird_conf.h b/bacula/src/dird/dird_conf.h index 67aca36078..5174a7a148 100644 --- a/bacula/src/dird/dird_conf.h +++ b/bacula/src/dird/dird_conf.h @@ -293,7 +293,7 @@ public: void create_client_globals(); int32_t getNumConcurrentJobs(); void setNumConcurrentJobs(int32_t num); - char *address(); + char *address(POOLMEM *&buf); void setAddress(char *addr); bool is_enabled(); void setEnabled(bool val); diff --git a/bacula/src/dird/fd_cmds.c b/bacula/src/dird/fd_cmds.c index d3cdb68859..1b64be4cd4 100644 --- a/bacula/src/dird/fd_cmds.c +++ b/bacula/src/dird/fd_cmds.c @@ -93,6 +93,7 @@ int connect_to_file_daemon(JCR *jcr, int retry_interval, int max_retry_time, if (!is_bsock_open(jcr->file_bsock)) { char name[MAX_NAME_LENGTH + 100]; + POOL_MEM buf; if (!fd) { fd = jcr->file_bsock = new_bsock(); @@ -101,8 +102,13 @@ int connect_to_file_daemon(JCR *jcr, int retry_interval, int max_retry_time, bstrncat(name, jcr->client->name(), sizeof(name)); fd->set_source_address(director->DIRsrc_addr); - if (!fd->connect(jcr,retry_interval,max_retry_time, heart_beat, name, jcr->client->address(), - NULL, jcr->client->FDport, verbose)) { + if (!fd->connect(jcr,retry_interval, + max_retry_time, + heart_beat, name, + jcr->client->address(buf.addr()), + NULL, + jcr->client->FDport, + verbose)) { fd->close(); jcr->setJobStatus(JS_ErrorTerminated); return 0; diff --git a/bacula/src/dird/snapshot.c b/bacula/src/dird/snapshot.c index 3dc3b6d10b..5aa7a76988 100644 --- a/bacula/src/dird/snapshot.c +++ b/bacula/src/dird/snapshot.c @@ -149,6 +149,7 @@ bool send_snapshot_retention(JCR *jcr, utime_t val) /* Called from delete_cmd() in ua_cmd.c */ int delete_snapshot(UAContext *ua) { + POOL_MEM buf; POOLMEM *out; SNAPSHOT_DBR snapdbr; CLIENT *client; @@ -178,7 +179,7 @@ int delete_snapshot(UAContext *ua) /* Try to connect for 15 seconds */ ua->send_msg(_("Connecting to Client %s at %s:%d\n"), - client->name(), client->address(), client->FDport); + client->name(), client->address(buf.addr()), client->FDport); if (!connect_to_file_daemon(ua->jcr, 1, 15, 0)) { ua->error_msg(_("Failed to connect to Client.\n")); ua->jcr->client = NULL; @@ -210,6 +211,7 @@ int delete_snapshot(UAContext *ua) */ int list_snapshot(UAContext *ua, alist *snap_list) { + POOL_MEM tmp; SNAPSHOT_DBR snap; POOLMEM *buf; CLIENT *client; @@ -225,7 +227,8 @@ int list_snapshot(UAContext *ua, alist *snap_list) /* Try to connect for 15 seconds */ ua->send_msg(_("Connecting to Client %s at %s:%d\n"), - client->name(), client->address(), client->FDport); + client->name(), client->address(tmp.addr()), client->FDport); + if (!connect_to_file_daemon(ua->jcr, 1, 15, 0)) { ua->error_msg(_("Failed to connect to Client.\n")); return 0; @@ -276,6 +279,7 @@ int prune_snapshot(UAContext *ua) CLIENT *client = NULL; BSOCK *fd = NULL; POOLMEM *buf = NULL; + POOL_MEM tmp; SNAPSHOT_DBR snapdbr; alist *lst; intptr_t id; @@ -318,7 +322,7 @@ int prune_snapshot(UAContext *ua) /* Try to connect for 15 seconds */ ua->send_msg(_("Connecting to Client %s at %s:%d\n"), - client->name(), client->address(), client->FDport); + client->name(), client->address(tmp.addr()), client->FDport); if (!connect_to_file_daemon(ua->jcr, 1, 15, 0)) { ua->error_msg(_("Failed to connect to Client.\n")); free_bsock(ua->jcr->file_bsock); diff --git a/bacula/src/dird/ua_cmds.c b/bacula/src/dird/ua_cmds.c index 870cc7691c..4ff0542950 100644 --- a/bacula/src/dird/ua_cmds.c +++ b/bacula/src/dird/ua_cmds.c @@ -695,6 +695,7 @@ extern char *configfile; static int setbwlimit_client(UAContext *ua, CLIENT *client, char *Job, int64_t limit) { + POOL_MEM buf; CLIENT *old_client; char ed1[50]; if (!client) { @@ -708,7 +709,7 @@ static int setbwlimit_client(UAContext *ua, CLIENT *client, char *Job, int64_t l /* Try to connect for 15 seconds */ ua->send_msg(_("Connecting to Client %s at %s:%d\n"), - client->name(), client->address(), client->FDport); + client->name(), client->address(buf.addr()), client->FDport); if (!connect_to_file_daemon(ua->jcr, 1, 15, 0)) { ua->error_msg(_("Failed to connect to Client.\n")); goto bail_out; @@ -814,8 +815,8 @@ static int setip_cmd(UAContext *ua, const char *cmd) } /* MA Bug 6 remove ifdef */ sockaddr_to_ascii(&(ua->UA_sock->client_addr), - sizeof(ua->UA_sock->client_addr), addr, sizeof(addr)); - client->setAddress(bstrdup(addr)); + sizeof(ua->UA_sock->client_addr), addr, sizeof(addr)); + client->setAddress(addr); ua->send_msg(_("Client \"%s\" address set to %s\n"), client->name(), addr); get_out: @@ -988,6 +989,7 @@ static void do_client_setdebug(UAContext *ua, CLIENT *client, int64_t level, int trace, int hangup, int blowup, char *options, char *tags) { + POOL_MEM buf; CLIENT *old_client; BSOCK *fd; @@ -997,7 +999,8 @@ static void do_client_setdebug(UAContext *ua, CLIENT *client, ua->jcr->client = client; /* Try to connect for 15 seconds */ ua->send_msg(_("Connecting to Client %s at %s:%d\n"), - client->name(), client->address(), client->FDport); + client->name(), client->address(buf.addr()), client->FDport); + if (!connect_to_file_daemon(ua->jcr, 1, 15, 0)) { ua->error_msg(_("Failed to connect to Client.\n")); ua->jcr->client = old_client; @@ -1029,6 +1032,7 @@ static void do_all_setdebug(UAContext *ua, int64_t level, { STORE *store, **unique_store; CLIENT *client, **unique_client; + POOL_MEM buf1, buf2; int i, j, found; int64_t t=0; @@ -1086,7 +1090,7 @@ static void do_all_setdebug(UAContext *ua, int64_t level, while ((client = (CLIENT *)GetNextRes(R_CLIENT, (RES *)client))) { found = 0; for (j=0; jaddress(), client->address()) == 0 && + if (strcmp(unique_client[j]->address(buf1.addr()), client->address(buf2.addr())) == 0 && unique_client[j]->FDport == client->FDport) { found = 1; break; @@ -1094,7 +1098,7 @@ static void do_all_setdebug(UAContext *ua, int64_t level, } if (!found) { unique_client[i++] = client; - Dmsg2(140, "Stuffing: %s:%d\n", client->address(), client->FDport); + Dmsg2(140, "Stuffing: %s:%d\n", client->address(buf1.addr()), client->FDport); } } UnlockRes(); @@ -1306,6 +1310,7 @@ static int estimate_cmd(UAContext *ua, const char *cmd) JOB *job = NULL; CLIENT *client = NULL; FILESET *fileset = NULL; + POOL_MEM buf; int listing = 0; char since[MAXSTRING]; JCR *jcr = ua->jcr; @@ -1447,7 +1452,7 @@ static int estimate_cmd(UAContext *ua, const char *cmd) get_level_since_time(ua->jcr, since, sizeof(since)); ua->send_msg(_("Connecting to Client %s at %s:%d\n"), - jcr->client->name(), jcr->client->address(), jcr->client->FDport); + jcr->client->name(), jcr->client->address(buf.addr()), jcr->client->FDport); if (!connect_to_file_daemon(jcr, 1, 15, 0)) { ua->error_msg(_("Failed to connect to Client.\n")); return 1; diff --git a/bacula/src/dird/ua_dotcmds.c b/bacula/src/dird/ua_dotcmds.c index f684e3b094..5ff3a91a5e 100644 --- a/bacula/src/dird/ua_dotcmds.c +++ b/bacula/src/dird/ua_dotcmds.c @@ -196,6 +196,7 @@ bool do_a_dot_command(UAContext *ua) */ static bool dot_ls_cmd(UAContext *ua, const char *cmd) { + POOL_MEM buf; CLIENT *client = NULL; char *path = NULL; JCR *jcr = ua->jcr; @@ -235,7 +236,7 @@ static bool dot_ls_cmd(UAContext *ua, const char *cmd) init_jcr_job_record(jcr); // need job ua->send_msg(_("Connecting to Client %s at %s:%d\n"), - jcr->client->name(), jcr->client->address(), jcr->client->FDport); + jcr->client->name(), jcr->client->address(buf.addr()), jcr->client->FDport); if (!connect_to_file_daemon(jcr, 1, 15, 0)) { ua->error_msg(_("Failed to connect to Client.\n")); @@ -1267,13 +1268,13 @@ static void do_storage_cmd(UAContext *ua, STORE *store, const char *cmd) static void do_client_cmd(UAContext *ua, CLIENT *client, const char *cmd) { BSOCK *fd; - + POOL_MEM buf; /* Connect to File daemon */ ua->jcr->client = client; /* Try to connect for 15 seconds */ ua->send_msg(_("Connecting to Client %s at %s:%d\n"), - client->name(), client->address(), client->FDport); + client->name(), client->address(buf.addr()), client->FDport); if (!connect_to_file_daemon(ua->jcr, 1, 15, 0)) { ua->error_msg(_("Failed to connect to Client.\n")); return; @@ -1659,6 +1660,7 @@ static bool clientscmd(UAContext *ua, const char *cmd) bool found=false; alist *clientlist = NULL; TmpClient *elt; + POOL_MEM buf; if ((i = find_arg_with_value(ua, "address")) >= 0) { ip = ua->argv[i]; @@ -1673,7 +1675,7 @@ static bool clientscmd(UAContext *ua, const char *cmd) foreach_res(client, R_CLIENT) { if (acl_access_client_ok(ua, client->name(), JT_BACKUP_RESTORE)) { if (ip) { - elt = new TmpClient(client->name(), client->address()); + elt = new TmpClient(client->name(), client->address(buf.addr())); clientlist->append(elt); } else { @@ -2120,8 +2122,9 @@ static bool defaultscmd(UAContext *ua, const char *cmd) } CLIENT *client = (CLIENT *)GetResWithName(R_CLIENT, ua->argv[1]); if (client) { + POOL_MEM buf; ua->send_msg("client=%s", client->name()); - ua->send_msg("address=%s", client->address()); + ua->send_msg("address=%s", client->address(buf.addr())); ua->send_msg("fdport=%d", client->FDport); ua->send_msg("file_retention=%s", edit_uint64(client->FileRetention, ed1)); ua->send_msg("job_retention=%s", edit_uint64(client->JobRetention, ed1)); diff --git a/bacula/src/dird/ua_status.c b/bacula/src/dird/ua_status.c index 03f55e9316..0aee6ab57a 100644 --- a/bacula/src/dird/ua_status.c +++ b/bacula/src/dird/ua_status.c @@ -123,6 +123,7 @@ static int do_network_status(UAContext *ua) char *store_address, ed1[50]; uint32_t store_port; uint64_t nb = 50 * 1024 * 1024; + POOL_MEM buf; int i = find_arg_with_value(ua, "bytes"); if (i > 0) { @@ -170,7 +171,7 @@ static int do_network_status(UAContext *ua) if (!ua->api) { ua->send_msg(_("Connecting to Client %s at %s:%d\n"), - client->name(), client->address(), client->FDport); + client->name(), client->address(buf.addr()), client->FDport); } if (!connect_to_file_daemon(jcr, 1, 15, 0)) { @@ -345,6 +346,7 @@ static void do_all_status(UAContext *ua) { STORE *store, **unique_store; CLIENT *client, **unique_client; + POOL_MEM buf1, buf2; int i, j; bool found; @@ -399,7 +401,7 @@ static void do_all_status(UAContext *ua) continue; } for (j=0; jaddress(), client->address()) == 0 && + if (strcmp(unique_client[j]->address(buf1.addr()), client->address(buf2.addr())) == 0 && unique_client[j]->FDport == client->FDport) { found = true; break; @@ -407,7 +409,7 @@ static void do_all_status(UAContext *ua) } if (!found) { unique_client[i++] = client; - Dmsg2(40, "Stuffing: %s:%d\n", client->address(), client->FDport); + Dmsg2(40, "Stuffing: %s:%d\n", client->address(buf1.addr()), client->FDport); } } UnlockRes(); @@ -590,6 +592,7 @@ static void do_storage_status(UAContext *ua, STORE *store, char *cmd) static void do_client_status(UAContext *ua, CLIENT *client, char *cmd) { BSOCK *fd; + POOL_MEM buf; if (!acl_access_client_ok(ua, client->name(), JT_BACKUP_RESTORE)) { ua->error_msg(_("No authorization for Client \"%s\"\n"), client->name()); @@ -606,7 +609,7 @@ static void do_client_status(UAContext *ua, CLIENT *client, char *cmd) /* Try to connect for 15 seconds */ if (!ua->api) ua->send_msg(_("Connecting to Client %s at %s:%d\n"), - client->name(), client->address(), client->FDport); + client->name(), client->address(buf.addr()), client->FDport); if (!connect_to_file_daemon(ua->jcr, 1, 15, 0)) { ua->send_msg(_("Failed to connect to Client %s.\n====\n"), client->name()); diff --git a/bacula/src/lib/util.c b/bacula/src/lib/util.c index 12b3d35420..2c425aa4cc 100644 --- a/bacula/src/lib/util.c +++ b/bacula/src/lib/util.c @@ -865,7 +865,7 @@ POOLMEM *edit_job_codes(JCR *jcr, char *omsg, char *imsg, const char *to, job_co char *p, *q; const char *str; char add[50]; - char name[MAX_NAME_LENGTH]; + char name[MAX_ESCAPE_NAME_LENGTH]; int i; *omsg = 0; @@ -976,7 +976,7 @@ POOLMEM *edit_job_codes(JCR *jcr, char *omsg, char *imsg, const char *to, job_co default: str = NULL; if (callback != NULL) { - str = callback(jcr, p, add, sizeof(add)); + str = callback(jcr, p, name, sizeof(name)); } if (!str) { -- 2.39.2