From 93d4e355d87ed6211031d22cd67f229246ec98ad Mon Sep 17 00:00:00 2001 From: rgirada Date: Fri, 24 Mar 2023 10:09:42 +0000 Subject: [PATCH 1/2] mgmtd: Fixing code coverity issues in mgmtd Description: the following list of coverity issues seen in mgmtd code. 1. CID 1536832: Memory - corruptions (OVERLAPPING_COPY) /mgmtd/mgmt_history.c: 85 in mgmt_history_create_cmt_rec() 2. CID 1536831: Error handling issues (NEGATIVE_RETURNS) /mgmtd/mgmt_be_server.c: 123 in mgmt_be_server_start() 3. CID 1536830: Resource leaks (RESOURCE_LEAK) /mgmtd/mgmt_history.c: 146 in mgmt_history_read_cmt_record_index() 4. CID 1536829: Error handling issues (NEGATIVE_RETURNS) /mgmtd/mgmt_fe_server.c: 123 in mgmt_fe_server_start() 5. CID 1536828: Possible Control flow issues (DEADCODE) /mgmtd/mgmt_txn.c: 1859 in mgmt_txn_get_config() 6. CID 1536827: Null pointer dereferences (NULL_RETURNS) /mgmtd/mgmt_ds.c: 526 in mgmt_ds_delete_data_nodes() Signed-off-by: Rajesh Girada --- mgmtd/mgmt_be_server.c | 2 +- mgmtd/mgmt_ds.c | 4 ++-- mgmtd/mgmt_fe_server.c | 2 +- mgmtd/mgmt_history.c | 2 +- mgmtd/mgmt_txn.c | 40 +++++----------------------------------- 5 files changed, 10 insertions(+), 40 deletions(-) diff --git a/mgmtd/mgmt_be_server.c b/mgmtd/mgmt_be_server.c index 0fa7ddd6d6..aa77464524 100644 --- a/mgmtd/mgmt_be_server.c +++ b/mgmtd/mgmt_be_server.c @@ -119,7 +119,7 @@ static void mgmt_be_server_start(const char *hostname) return; mgmt_be_server_start_failed: - if (sock) + if (sock > 0) close(sock); mgmt_be_listen_fd = -1; diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c index 10b3cecb92..b41b9d23d3 100644 --- a/mgmtd/mgmt_ds.c +++ b/mgmtd/mgmt_ds.c @@ -499,12 +499,12 @@ int mgmt_ds_delete_data_nodes(struct mgmt_ds_ctx *ds_ctx, const char *xpath) */ return NB_ERR_NOT_FOUND; /* destroy dependant */ - if (nb_node->dep_cbs.get_dependant_xpath) { + if (nb_node && nb_node->dep_cbs.get_dependant_xpath) { nb_node->dep_cbs.get_dependant_xpath(dnode, dep_xpath); dep_dnode = yang_dnode_get( ds_ctx->config_ds ? ds_ctx->root.cfg_root->dnode - : ds_ctx->root.dnode_root, + : ds_ctx->root.dnode_root, dep_xpath); if (dep_dnode) lyd_free_tree(dep_dnode); diff --git a/mgmtd/mgmt_fe_server.c b/mgmtd/mgmt_fe_server.c index 6097c23aac..e8bbe139bb 100644 --- a/mgmtd/mgmt_fe_server.c +++ b/mgmtd/mgmt_fe_server.c @@ -119,7 +119,7 @@ static void mgmt_fe_server_start(const char *hostname) return; mgmt_fe_server_start_failed: - if (sock) + if (sock > 0) close(sock); mgmt_fe_listen_fd = -1; diff --git a/mgmtd/mgmt_history.c b/mgmtd/mgmt_history.c index 6f9f2dd63f..533b68628e 100644 --- a/mgmtd/mgmt_history.c +++ b/mgmtd/mgmt_history.c @@ -82,7 +82,7 @@ static struct mgmt_cmt_info_t *mgmt_history_create_cmt_rec(void) mgmt_realtime_to_string(&cmt_recd_tv, new->time_str, sizeof(new->time_str)); mgmt_history_hash(new->time_str, new->cmtid_str); - snprintf(new->cmt_json_file, sizeof(new->cmt_json_file), + snprintf(new->cmt_json_file, sizeof(new->cmt_json_file) - 1, MGMTD_COMMIT_FILE_PATH, new->cmtid_str); if (mgmt_cmt_infos_count(&mm->cmts) == MGMTD_MAX_COMMIT_LIST) { diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index 5fa8aabfd6..25943ff742 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -1787,27 +1787,10 @@ static int mgmt_txn_get_config(struct mgmt_txn_ctx *txn, struct mgmt_txn_req *txn_req, struct mgmt_ds_ctx *ds_ctx) { - struct mgmt_txn_reqs_head *req_list = NULL; - struct mgmt_txn_reqs_head *pending_list = NULL; int indx; struct mgmt_get_data_req *get_data; struct mgmt_get_data_reply *get_reply; - switch (txn_req->req_event) { - case MGMTD_TXN_PROC_GETCFG: - req_list = &txn->get_cfg_reqs; - break; - case MGMTD_TXN_PROC_GETDATA: - req_list = &txn->get_data_reqs; - break; - case MGMTD_TXN_PROC_SETCFG: - case MGMTD_TXN_PROC_COMMITCFG: - case MGMTD_TXN_COMMITCFG_TIMEOUT: - case MGMTD_TXN_CLEANUP: - assert(!"Wrong txn request type!"); - break; - } - get_data = txn_req->req.get_data; if (!get_data->reply) { @@ -1852,24 +1835,11 @@ static int mgmt_txn_get_config(struct mgmt_txn_ctx *txn, mgmt_txn_get_config_failed: - if (pending_list) { - /* - * Move the transaction to corresponding pending list. - */ - if (req_list) - mgmt_txn_reqs_del(req_list, txn_req); - txn_req->pending_be_proc = true; - mgmt_txn_reqs_add_tail(pending_list, txn_req); - MGMTD_TXN_DBG( - "Moved Req: %p for Txn: %p from Req-List to Pending-List", - txn_req, txn_req->txn); - } else { - /* - * Delete the txn request. It will also remove it from request - * list. - */ - mgmt_txn_req_free(&txn_req); - } + /* + * Delete the txn request. It will also remove it from request + * list. + */ + mgmt_txn_req_free(&txn_req); return 0; } From 83b78f43f4e2abfdc1d4f841e9b40694e08eff01 Mon Sep 17 00:00:00 2001 From: rgirada Date: Tue, 28 Mar 2023 11:13:47 +0000 Subject: [PATCH 2/2] mgmtd: Fixing style warnings Description: Fixing the style warnings in the mgmtd code. Signed-off-by: Rajesh Girada --- lib/mgmt_be_client.c | 15 ++++++++------- lib/mgmt_fe_client.c | 29 ++++++++++++++++------------- mgmtd/mgmt_be_adapter.c | 12 ++++++------ mgmtd/mgmt_ds.c | 1 + mgmtd/mgmt_fe_adapter.c | 26 +++++++++++++++----------- mgmtd/mgmt_history.c | 15 ++++++++++----- mgmtd/mgmt_history.h | 10 +++++----- mgmtd/mgmt_txn.c | 1 + mgmtd/mgmt_vty.c | 2 ++ 9 files changed, 64 insertions(+), 47 deletions(-) diff --git a/lib/mgmt_be_client.c b/lib/mgmt_be_client.c index 36c78052bc..7437eedfc7 100644 --- a/lib/mgmt_be_client.c +++ b/lib/mgmt_be_client.c @@ -16,17 +16,17 @@ #include "sockopt.h" #ifdef REDIRECT_DEBUG_TO_STDERR -#define MGMTD_BE_CLIENT_DBG(fmt, ...) \ +#define MGMTD_BE_CLIENT_DBG(fmt, ...) \ fprintf(stderr, "%s: " fmt "\n", __func__, ##__VA_ARGS__) -#define MGMTD_BE_CLIENT_ERR(fmt, ...) \ +#define MGMTD_BE_CLIENT_ERR(fmt, ...) \ fprintf(stderr, "%s: ERROR, " fmt "\n", __func__, ##__VA_ARGS__) #else /* REDIRECT_DEBUG_TO_STDERR */ -#define MGMTD_BE_CLIENT_DBG(fmt, ...) \ +#define MGMTD_BE_CLIENT_DBG(fmt, ...) \ do { \ - if (mgmt_debug_be_client) \ - zlog_debug("%s: " fmt, __func__, ##__VA_ARGS__); \ + if (mgmt_debug_be_client) \ + zlog_debug("%s: " fmt, __func__, ##__VA_ARGS__); \ } while (0) -#define MGMTD_BE_CLIENT_ERR(fmt, ...) \ +#define MGMTD_BE_CLIENT_ERR(fmt, ...) \ zlog_err("%s: ERROR: " fmt, __func__, ##__VA_ARGS__) #endif /* REDIRECT_DEBUG_TO_STDERR */ @@ -597,7 +597,8 @@ static int mgmt_be_txn_cfg_prepare(struct mgmt_be_txn_ctx *txn) MGMTD_BE_CLIENT_DBG( "Avg-nb-edit-duration %lu uSec, nb-prep-duration %lu (avg: %lu) uSec, batch size %u", client_ctx->avg_edit_nb_cfg_tm, prep_nb_cfg_tm, - client_ctx->avg_prep_nb_cfg_tm, (uint32_t)num_processed); + client_ctx->avg_prep_nb_cfg_tm, + (uint32_t)num_processed); if (error) mgmt_be_txn_cfg_abort(txn); diff --git a/lib/mgmt_fe_client.c b/lib/mgmt_fe_client.c index a2c4fd6572..7cb9aa3def 100644 --- a/lib/mgmt_fe_client.c +++ b/lib/mgmt_fe_client.c @@ -109,8 +109,8 @@ mgmt_fe_find_session_by_session_id(struct mgmt_fe_client_ctx *client_ctx, FOREACH_SESSION_IN_LIST (client_ctx, session) { if (session->session_id == session_id) { MGMTD_FE_CLIENT_DBG( - "Found session %p for session-id %llu.", session, - (unsigned long long)session_id); + "Found session %p for session-id %llu.", + session, (unsigned long long)session_id); return session; } } @@ -274,7 +274,8 @@ mgmt_fe_send_lockds_req(struct mgmt_fe_client_ctx *client_ctx, MGMTD_FE_CLIENT_DBG( "Sending %sLOCK_REQ message for Ds:%d session %llu to MGMTD Frontend server", - lock ? "" : "UN", ds_id, (unsigned long long)session->client_id); + lock ? "" : "UN", ds_id, + (unsigned long long)session->client_id); return mgmt_fe_client_send_msg(client_ctx, &fe_msg); } @@ -310,12 +311,12 @@ mgmt_fe_send_setcfg_req(struct mgmt_fe_client_ctx *client_ctx, return mgmt_fe_client_send_msg(client_ctx, &fe_msg); } -static int -mgmt_fe_send_commitcfg_req(struct mgmt_fe_client_ctx *client_ctx, - struct mgmt_fe_client_session *session, - uint64_t req_id, Mgmtd__DatastoreId src_ds_id, - Mgmtd__DatastoreId dest_ds_id, bool validate_only, - bool abort) +static int mgmt_fe_send_commitcfg_req(struct mgmt_fe_client_ctx *client_ctx, + struct mgmt_fe_client_session *session, + uint64_t req_id, + Mgmtd__DatastoreId src_ds_id, + Mgmtd__DatastoreId dest_ds_id, + bool validate_only, bool abort) { (void)req_id; Mgmtd__FeMessage fe_msg; @@ -450,15 +451,17 @@ mgmt_fe_client_handle_msg(struct mgmt_fe_client_ctx *client_ctx, if (session && fe_msg->session_reply->success) { MGMTD_FE_CLIENT_DBG( "Session Create for client-id %llu successful.", - (unsigned long long)fe_msg - ->session_reply->client_conn_id); + (unsigned long long) + fe_msg->session_reply + ->client_conn_id); session->session_id = fe_msg->session_reply->session_id; } else { MGMTD_FE_CLIENT_ERR( "Session Create for client-id %llu failed.", - (unsigned long long)fe_msg - ->session_reply->client_conn_id); + (unsigned long long) + fe_msg->session_reply + ->client_conn_id); } } else if (!fe_msg->session_reply->create) { MGMTD_FE_CLIENT_DBG( diff --git a/mgmtd/mgmt_be_adapter.c b/mgmtd/mgmt_be_adapter.c index af43cf3eae..cf7d705943 100644 --- a/mgmtd/mgmt_be_adapter.c +++ b/mgmtd/mgmt_be_adapter.c @@ -80,23 +80,23 @@ static const struct mgmt_be_xpath_map_reg xpath_static_map_reg[] = { .be_clients = (enum mgmt_be_client_id[]){ #if HAVE_STATICD - MGMTD_BE_CLIENT_ID_STATICD, + MGMTD_BE_CLIENT_ID_STATICD, #endif MGMTD_BE_CLIENT_ID_MAX}}, {.xpath_regexp = "/frr-interface:lib/*", .be_clients = (enum mgmt_be_client_id[]){ #if HAVE_STATICD - MGMTD_BE_CLIENT_ID_STATICD, + MGMTD_BE_CLIENT_ID_STATICD, #endif MGMTD_BE_CLIENT_ID_MAX}}, {.xpath_regexp = - "/frr-routing:routing/control-plane-protocols/control-plane-protocol[type='frr-staticd:staticd'][name='staticd'][vrf='default']/frr-staticd:staticd/*", + "/frr-routing:routing/control-plane-protocols/control-plane-protocol[type='frr-staticd:staticd'][name='staticd'][vrf='default']/frr-staticd:staticd/*", .be_clients = (enum mgmt_be_client_id[]){ #if HAVE_STATICD - MGMTD_BE_CLIENT_ID_STATICD, + MGMTD_BE_CLIENT_ID_STATICD, #endif MGMTD_BE_CLIENT_ID_MAX}}, }; @@ -347,8 +347,8 @@ mgmt_be_adapter_cleanup_old_conn(struct mgmt_be_client_adapter *adapter) struct mgmt_be_client_adapter *old; FOREACH_ADAPTER_IN_LIST (old) { - if (old != adapter - && !strncmp(adapter->name, old->name, sizeof(adapter->name))) { + if (old != adapter && + !strncmp(adapter->name, old->name, sizeof(adapter->name))) { /* * We have a Zombie lingering around */ diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c index b41b9d23d3..58c49b8789 100644 --- a/mgmtd/mgmt_ds.c +++ b/mgmtd/mgmt_ds.c @@ -174,6 +174,7 @@ static int mgmt_ds_load_cfg_from_file(const char *filepath, void mgmt_ds_reset_candidate(void) { struct lyd_node *dnode = mm->candidate_ds->root.cfg_root->dnode; + if (dnode) yang_dnode_free(dnode); diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index 5826b17de7..90e6870fd4 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -495,7 +495,8 @@ static int mgmt_fe_send_setcfg_reply(struct mgmt_fe_session_ctx *session, if (implicit_commit) { if (mm->perf_stats_en) - gettimeofday(&session->adapter->cmt_stats.last_end, NULL); + gettimeofday(&session->adapter->cmt_stats.last_end, + NULL); mgmt_fe_session_compute_commit_timers( &session->adapter->cmt_stats); } @@ -715,8 +716,8 @@ mgmt_fe_adapter_cleanup_old_conn(struct mgmt_fe_client_adapter *adapter) struct mgmt_fe_client_adapter *old; FOREACH_ADAPTER_IN_LIST (old) { - if (old != adapter - && !strncmp(adapter->name, old->name, sizeof(adapter->name))) { + if (old != adapter && + !strncmp(adapter->name, old->name, sizeof(adapter->name))) { /* * We have a Zombie lingering around */ @@ -1109,8 +1110,8 @@ mgmt_fe_session_handle_getdata_req_msg(struct mgmt_fe_session_ctx *session, MGMTD_TXN_TYPE_SHOW); if (session->txn_id == MGMTD_SESSION_ID_NONE) { mgmt_fe_send_getdata_reply( - session, getdata_req->ds_id, getdata_req->req_id, - false, NULL, + session, getdata_req->ds_id, + getdata_req->req_id, false, NULL, "Failed to create a Show transaction!"); goto mgmt_fe_sess_handle_getdata_req_failed; } @@ -1780,8 +1781,8 @@ mgmt_fe_adapter_cmt_stats_write(struct vty *vty, sizeof(buf))); vty_out(vty, " Apply-Config Start: \t\t%s\n", mgmt_realtime_to_string( - &adapter->cmt_stats.apply_cfg_start, buf, - sizeof(buf))); + &adapter->cmt_stats.apply_cfg_start, + buf, sizeof(buf))); vty_out(vty, " Apply-Config End: \t\t%s\n", mgmt_realtime_to_string( &adapter->cmt_stats.apply_cfg_end, buf, @@ -1818,8 +1819,9 @@ mgmt_fe_adapter_setcfg_stats_write(struct vty *vty, adapter->setcfg_stats.avg_tm); vty_out(vty, " Last-Set-Cfg-Details:\n"); vty_out(vty, " Set-Cfg Start: \t\t\t%s\n", - mgmt_realtime_to_string(&adapter->setcfg_stats.last_start, - buf, sizeof(buf))); + mgmt_realtime_to_string( + &adapter->setcfg_stats.last_start, buf, + sizeof(buf))); vty_out(vty, " Set-Cfg End: \t\t\t%s\n", mgmt_realtime_to_string(&adapter->setcfg_stats.last_end, buf, sizeof(buf))); @@ -1894,9 +1896,11 @@ void mgmt_fe_adapter_reset_perf_stats(struct vty *vty) struct mgmt_fe_session_ctx *session; FOREACH_ADAPTER_IN_LIST (adapter) { - memset(&adapter->setcfg_stats, 0, sizeof(adapter->setcfg_stats)); + memset(&adapter->setcfg_stats, 0, + sizeof(adapter->setcfg_stats)); FOREACH_SESSION_IN_LIST (adapter, session) { - memset(&adapter->cmt_stats, 0, sizeof(adapter->cmt_stats)); + memset(&adapter->cmt_stats, 0, + sizeof(adapter->cmt_stats)); } } } diff --git a/mgmtd/mgmt_history.c b/mgmtd/mgmt_history.c index 533b68628e..2251c49f1c 100644 --- a/mgmtd/mgmt_history.c +++ b/mgmtd/mgmt_history.c @@ -33,7 +33,7 @@ DECLARE_DLIST(mgmt_cmt_infos, struct mgmt_cmt_info_t, cmts); * The only instance of VTY session that has triggered an ongoing * config rollback operation. */ -static struct vty *rollback_vty = NULL; +static struct vty *rollback_vty; static bool mgmt_history_record_exists(char *file_path) { @@ -100,7 +100,8 @@ static struct mgmt_cmt_info_t *mgmt_history_create_cmt_rec(void) return new; } -static struct mgmt_cmt_info_t *mgmt_history_find_cmt_record(const char *cmtid_str) +static struct mgmt_cmt_info_t * +mgmt_history_find_cmt_record(const char *cmtid_str) { struct mgmt_cmt_info_t *cmt_info; @@ -129,7 +130,8 @@ static bool mgmt_history_read_cmt_record_index(void) while ((fread(&cmt_info, sizeof(cmt_info), 1, fp)) > 0) { if (cnt < MGMTD_MAX_COMMIT_LIST) { - if (!mgmt_history_record_exists(cmt_info.cmt_json_file)) { + if (!mgmt_history_record_exists( + cmt_info.cmt_json_file)) { zlog_err( "Commit record present in index_file, but commit file %s missing", cmt_info.cmt_json_file); @@ -282,7 +284,8 @@ int mgmt_history_rollback_by_id(struct vty *vty, const char *cmtid_str) FOREACH_CMT_REC (mm, cmt_info) { if (strncmp(cmt_info->cmtid_str, cmtid_str, MGMTD_MD5_HASH_STR_HEX_LEN) == 0) { - ret = mgmt_history_rollback_to_cmt(vty, cmt_info, false); + ret = mgmt_history_rollback_to_cmt(vty, cmt_info, + false); return ret; } @@ -321,7 +324,8 @@ int mgmt_history_rollback_n(struct vty *vty, int num_cmts) FOREACH_CMT_REC (mm, cmt_info) { if (cnt == num_cmts) { - ret = mgmt_history_rollback_to_cmt(vty, cmt_info, false); + ret = mgmt_history_rollback_to_cmt(vty, cmt_info, + false); return ret; } @@ -356,6 +360,7 @@ void show_mgmt_cmt_history(struct vty *vty) void mgmt_history_new_record(struct mgmt_ds_ctx *ds_ctx) { struct mgmt_cmt_info_t *cmt_info = mgmt_history_create_cmt_rec(); + mgmt_ds_dump_ds_to_file(cmt_info->cmt_json_file, ds_ctx); mgmt_history_dump_cmt_record_index(); } diff --git a/mgmtd/mgmt_history.h b/mgmtd/mgmt_history.h index 29a1d7738e..5f96cf90e1 100644 --- a/mgmtd/mgmt_history.h +++ b/mgmtd/mgmt_history.h @@ -1,10 +1,10 @@ // SPDX-License-Identifier: GPL-2.0-or-later /* - * Copyright (C) 2021 Vmware, Inc. - * Pushpasis Sarkar - * Copyright (c) 2023, LabN Consulting, L.L.C. - * - */ + * Copyright (C) 2021 Vmware, Inc. + * Pushpasis Sarkar + * Copyright (c) 2023, LabN Consulting, L.L.C. + * + */ #ifndef _FRR_MGMTD_HISTORY_H_ #define _FRR_MGMTD_HISTORY_H_ diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index 25943ff742..2ba0cb413a 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -1258,6 +1258,7 @@ static int mgmt_txn_prepare_config(struct mgmt_txn_ctx *txn) char err_buf[1024] = {0}; nb_ctx.client = NB_CLIENT_MGMTD_SERVER; nb_ctx.user = (void *)txn; + ret = nb_candidate_validate_yang(nb_config, false, err_buf, sizeof(err_buf) - 1); if (ret != NB_OK) { diff --git a/mgmtd/mgmt_vty.c b/mgmtd/mgmt_vty.c index cb09544fdc..fa34d90c75 100644 --- a/mgmtd/mgmt_vty.c +++ b/mgmtd/mgmt_vty.c @@ -391,6 +391,7 @@ static struct cmd_node debug_node = { static int config_write_mgmt_debug_helper(struct vty *vty, bool config) { int n = mgmt_debug_be + mgmt_debug_fe + mgmt_debug_ds + mgmt_debug_txn; + if (!n) return 0; @@ -442,6 +443,7 @@ DEFPY(debug_mgmt, debug_mgmt_cmd, "Transaction debug\n") { bool set = !no; + if (all) be = fe = ds = txn = set ? all : NULL;