diff --git a/CHANGELOG.md b/CHANGELOG.md index 560857f1..5b2022be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,7 @@ Developers may need to change their code ### Minor features +* Added checks of changed handlers or blocked signal after plugin function calls * Added set/get pointer API to clixon_data: * clicon_ptr_get(), clicon_ptr_set(), * Restconf YANG PATCH according to RFC 8072 diff --git a/apps/backend/backend_commit.c b/apps/backend/backend_commit.c index 8abcbcbc..e5a141c7 100644 --- a/apps/backend/backend_commit.c +++ b/apps/backend/backend_commit.c @@ -937,10 +937,16 @@ from_client_restart_one(clicon_handle h, goto done; /* Application may define extra xml in its reset function*/ if ((resetfn = clixon_plugin_api_get(cp)->ca_reset) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if ((retval = resetfn(h, db)) < 0) { clicon_debug(1, "plugin_start() failed"); goto done; } + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } /* 1. Start transaction */ if ((td = transaction_new()) == NULL) diff --git a/apps/backend/backend_plugin.c b/apps/backend/backend_plugin.c index c8bff188..0d4565b7 100644 --- a/apps/backend/backend_plugin.c +++ b/apps/backend/backend_plugin.c @@ -82,12 +82,18 @@ clixon_plugin_reset_one(clixon_plugin_t *cp, plgreset_t *fn; /* callback */ if ((fn = clixon_plugin_api_get(cp)->ca_reset) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h, db) < 0) { if (clicon_errno < 0) clicon_log(LOG_WARNING, "%s: Internal error: Reset callback in plugin: %s returned -1 but did not make a clicon_err call", __FUNCTION__, clixon_plugin_name_get(cp)); goto done; } + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } retval = 0; done: @@ -133,6 +139,10 @@ clixon_plugin_pre_daemon_one(clixon_plugin_t *cp, plgdaemon_t *fn; /* Daemonize plugin callback function */ if ((fn = clixon_plugin_api_get(cp)->ca_pre_daemon) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h) < 0) { if (clicon_errno < 0) clicon_log(LOG_WARNING, "%s: Internal error: Pre-daemon callback in plugin:\ @@ -140,6 +150,8 @@ clixon_plugin_pre_daemon_one(clixon_plugin_t *cp, __FUNCTION__, clixon_plugin_name_get(cp)); goto done; } + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } retval = 0; done: @@ -186,12 +198,18 @@ clixon_plugin_daemon_one(clixon_plugin_t *cp, plgdaemon_t *fn; /* Daemonize plugin callback function */ if ((fn = clixon_plugin_api_get(cp)->ca_daemon) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h) < 0) { if (clicon_errno < 0) clicon_log(LOG_WARNING, "%s: Internal error: Daemon callback in plugin: %s returned -1 but did not make a clicon_err call", __FUNCTION__, clixon_plugin_name_get(cp)); goto done; } + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } retval = 0; done: @@ -265,12 +283,18 @@ clixon_plugin_statedata_one(clixon_plugin_t *cp, if ((fn = clixon_plugin_api_get(cp)->ca_statedata) != NULL){ if ((x = xml_new(DATASTORE_TOP_SYMBOL, NULL, CX_ELMNT)) == NULL) goto done; + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h, nsc, xpath, x) < 0){ if (clicon_errno < 0) clicon_log(LOG_WARNING, "%s: Internal error: State callback in plugin: %s returned -1 but did not make a clicon_err call", __FUNCTION__, clixon_plugin_name_get(cp)); goto fail; /* Dont quit here on user callbacks */ } + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } if (xp && x) *xp = x; @@ -410,8 +434,14 @@ clixon_plugin_lockdb_one(clixon_plugin_t *cp, plglockdb_t *fn; /* Plugin statedata fn */ if ((fn = clixon_plugin_api_get(cp)->ca_lockdb) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h, db, lock, id) < 0) goto done; + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } retval = 0; done: @@ -579,12 +609,18 @@ plugin_transaction_begin_one(clixon_plugin_t *cp, trans_cb_t *fn; if ((fn = clixon_plugin_api_get(cp)->ca_trans_begin) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h, (transaction_data)td) < 0){ if (!clicon_errno) /* sanity: log if clicon_err() is not called ! */ clicon_log(LOG_NOTICE, "%s: Plugin '%s' callback does not make clicon_err call on error", __FUNCTION__, clixon_plugin_name_get(cp)); goto done; } + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } retval = 0; done: @@ -632,12 +668,18 @@ plugin_transaction_validate_one(clixon_plugin_t *cp, trans_cb_t *fn; if ((fn = clixon_plugin_api_get(cp)->ca_trans_validate) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h, (transaction_data)td) < 0){ if (!clicon_errno) /* sanity: log if clicon_err() is not called ! */ clicon_log(LOG_NOTICE, "%s: Plugin '%s' callback does not make clicon_err call on error", __FUNCTION__, clixon_plugin_name_get(cp)); goto done; } + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } retval = 0; done: @@ -683,12 +725,18 @@ plugin_transaction_complete_one(clixon_plugin_t *cp, trans_cb_t *fn; if ((fn = clixon_plugin_api_get(cp)->ca_trans_complete) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h, (transaction_data)td) < 0){ if (!clicon_errno) /* sanity: log if clicon_err() is not called ! */ clicon_log(LOG_NOTICE, "%s: Plugin '%s' callback does not make clicon_err call on error", __FUNCTION__, clixon_plugin_name_get(cp)); goto done; } + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } retval = 0; done: @@ -766,12 +814,18 @@ plugin_transaction_commit_one(clixon_plugin_t *cp, trans_cb_t *fn; if ((fn = clixon_plugin_api_get(cp)->ca_trans_commit) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h, (transaction_data)td) < 0){ if (!clicon_errno) /* sanity: log if clicon_err() is not called ! */ clicon_log(LOG_NOTICE, "%s: Plugin '%s' callback does not make clicon_err call on error", __FUNCTION__, clixon_plugin_name_get(cp)); goto done; } + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } retval = 0; done: @@ -825,12 +879,18 @@ plugin_transaction_commit_done_one(clixon_plugin_t *cp, trans_cb_t *fn; if ((fn = clixon_plugin_api_get(cp)->ca_trans_commit_done) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h, (transaction_data)td) < 0){ if (!clicon_errno) /* sanity: log if clicon_err() is not called ! */ clicon_log(LOG_NOTICE, "%s: Plugin '%s' callback does not make clicon_err call on error", __FUNCTION__, clixon_plugin_name_get(cp)); goto done; } + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } retval = 0; done: @@ -876,12 +936,18 @@ plugin_transaction_end_one(clixon_plugin_t *cp, trans_cb_t *fn; if ((fn = clixon_plugin_api_get(cp)->ca_trans_end) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h, (transaction_data)td) < 0){ if (!clicon_errno) /* sanity: log if clicon_err() is not called ! */ clicon_log(LOG_NOTICE, "%s: Plugin '%s' callback does not make clicon_err call on error", __FUNCTION__, clixon_plugin_name_get(cp)); goto done; } + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } retval = 0; done: @@ -920,12 +986,18 @@ plugin_transaction_abort_one(clixon_plugin_t *cp, trans_cb_t *fn; if ((fn = clixon_plugin_api_get(cp)->ca_trans_abort) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h, (transaction_data)td) < 0){ if (!clicon_errno) /* sanity: log if clicon_err() is not called ! */ clicon_log(LOG_NOTICE, "%s: Plugin '%s' callback does not make clicon_err call on error", __FUNCTION__, clixon_plugin_name_get(cp)); goto done; } + if (plugin_context_check(&pc, clixon_plugin_name_get(cp), __FUNCTION__) < 0) + goto done; } retval = 0; done: diff --git a/apps/cli/cli_plugin.c b/apps/cli/cli_plugin.c index 89f9294f..34c1888b 100644 --- a/apps/cli/cli_plugin.c +++ b/apps/cli/cli_plugin.c @@ -522,6 +522,47 @@ cli_handler_err(FILE *f) return 0; } +/*! Variant of eval for context checking + * @see cligen_eval + */ +int +cligen_clixon_eval(cligen_handle h, + cg_obj *co, + cvec *cvv) +{ + struct cg_callback *cc; + int retval = 0; + cvec *argv; + + if (h) + cligen_co_match_set(h, co); + for (cc = co->co_callbacks; cc; cc=cc->cc_next){ + /* Vector cvec argument to callback */ + if (cc->cc_fn_vec){ + plugin_context_t pc = {0,}; + argv = cc->cc_cvec ? cvec_dup(cc->cc_cvec) : NULL; + cligen_fn_str_set(h, cc->cc_fn_str); + if (plugin_context_get(&pc) < 0) + break; + if ((retval = (*cc->cc_fn_vec)( + cligen_userhandle(h)?cligen_userhandle(h):h, + cvv, + argv)) < 0){ + if (argv != NULL) + cvec_free(argv); + cligen_fn_str_set(h, NULL); + break; + } + if (plugin_context_check(&pc, "CLIgen", cc->cc_fn_str) < 0) + break; + if (argv != NULL) + cvec_free(argv); + cligen_fn_str_set(h, NULL); + } + } + return retval; +} + /*! Evaluate a matched command * @param[in] h Clicon handle * @param[in] cmd The command string @@ -541,7 +582,8 @@ clicon_eval(clicon_handle h, cli_output_reset(); if (!cligen_exiting(cli_cligen(h))) { clicon_err_reset(); - if ((retval = cligen_eval(cli_cligen(h), match_obj, cvv)) < 0) { + + if ((retval = cligen_clixon_eval(cli_cligen(h), match_obj, cvv)) < 0) { #if 0 /* This is removed since we get two error messages on failure. But maybe only sometime? Both a real log when clicon_err is called, and the here again. diff --git a/lib/clixon/clixon_plugin.h b/lib/clixon/clixon_plugin.h index ffebc982..e487b035 100644 --- a/lib/clixon/clixon_plugin.h +++ b/lib/clixon/clixon_plugin.h @@ -337,6 +337,17 @@ struct clixon_plugin_api{ #define ca_trans_abort u.cau_backend.cb_trans_abort #define ca_datastore_upgrade u.cau_backend.cb_datastore_upgrade +/*! Structure for checking status before and after a plugin call + * Currently signal settings: blocked and handlers, could be extended to more + * @see plugin_context_check + */ +struct plugin_context { + sigset_t pc_sigset; /* See sigprocmask(2) */ + struct sigaction pc_sigaction_vec[32]; /* See sigaction(2) */ + int pc_status; /* 0: OK, -1: fail */ +}; +typedef struct plugin_context plugin_context_t; + /* * Macros */ @@ -375,6 +386,9 @@ int clixon_plugins_load(clicon_handle h, const char *function, const char *dir, int clixon_pseudo_plugin(clicon_handle h, const char *name, clixon_plugin_t **cpp); +int plugin_context_get(plugin_context_t *pc); +int plugin_context_check(plugin_context_t *pc, const char *name, const char *fn); + int clixon_plugin_start_one(clixon_plugin_t *cp, clicon_handle h); int clixon_plugin_start_all(clicon_handle h); diff --git a/lib/src/clixon_plugin.c b/lib/src/clixon_plugin.c index 20ea2a99..9a804cdd 100644 --- a/lib/src/clixon_plugin.c +++ b/lib/src/clixon_plugin.c @@ -43,6 +43,7 @@ #include #include #include +#include #include #include #include @@ -330,6 +331,7 @@ plugin_load_one(clicon_handle h, clixon_plugin_t *cp = NULL; char *name; char *p; + plugin_context_t pc = {0,}; clicon_debug(1, "%s file:%s function:%s", __FUNCTION__, file, function); dlerror(); /* Clear any existing error */ @@ -348,6 +350,11 @@ plugin_load_one(clicon_handle h, goto done; } clicon_err_reset(); + + + if (plugin_context_get(&pc) < 0) + goto done; + if ((api = initfn(h)) == NULL) { if (!clicon_errno){ /* if clicon_err() is not called then log and continue */ clicon_log(LOG_DEBUG, "Warning: failed to initiate %s", strrchr(file,'/')?strchr(file, '/'):file); @@ -359,6 +366,9 @@ plugin_load_one(clicon_handle h, goto done; } } + if (plugin_context_check(&pc, file, __FUNCTION__) < 0) + goto done; + /* Note: sizeof clixon_plugin_api which is largest of clixon_plugin_api:s */ if ((cp = (clixon_plugin_t *)malloc(sizeof(struct clixon_plugin))) == NULL){ clicon_err(OE_UNIX, errno, "malloc"); @@ -483,6 +493,93 @@ done: return retval; } +/*! Get system context, eg signal procmask (for blocking) and sigactions + * Call this before a plugin + * @see plugin_context_check + */ +int +plugin_context_get(plugin_context_t *pc) +{ + int retval = -1; + int i; + + if (sigprocmask(0, NULL, &pc->pc_sigset) < 0){ + clicon_err(OE_UNIX, errno, "sigprocmask"); + goto done; + } + for (i=1; i<32; i++){ + if (sigaction(i, NULL, &pc->pc_sigaction_vec[i]) < 0){ + clicon_err(OE_UNIX, errno, "sigaction"); + goto done; + } + /* Mask SA_RESTORER: Not intended for application use. + * Note that it may not be included in user space so may be hardcoded below + */ +#ifdef SA_RESTORER + pc->pc_sigaction_vec[i].sa_flags &= ~SA_RESTORER; +#else + pc->pc_sigaction_vec[i].sa_flags &= ~0x04000000; +#endif + } + retval = 0; + done: + return retval; +} + +/*! Given an existing, old plugin context, check if anytjing has changed + * @param[in,out] oldpc Old plugin context, status will be returned inside oldpc + * @param[in] name Name of plugin for logging + * @param[in] fn Name of callback + * @retval -1 Error + * @retval 0 OK + * @see plugin_context_get + */ +int +plugin_context_check(plugin_context_t *oldpc, + const char *name, + const char *fn) +{ + int retval = -1; + int failed; + int i; + plugin_context_t newpc = {0, }; + + if (plugin_context_get(&newpc) < 0) + goto done; + for (i=1; i<32; i++){ + failed = 0; + if (sigismember(&oldpc->pc_sigset, i) != sigismember(&newpc.pc_sigset, i)){ + clicon_log(LOG_WARNING, "%s Plugin %s %s: Changed blocking of signal %s(%d) from %d to %d", __FUNCTION__, + name, fn, strsignal(i), i, + sigismember(&oldpc->pc_sigset, i), + sigismember(&newpc.pc_sigset, i) + ); + failed++; + } + if (oldpc->pc_sigaction_vec[i].sa_flags != newpc.pc_sigaction_vec[i].sa_flags){ + clicon_log(LOG_WARNING, "%s Plugin %s %s: Changed flags of signal %s(%d) from 0x%x to 0x%x", __FUNCTION__, + name, fn, strsignal(i), i, + oldpc->pc_sigaction_vec[i].sa_flags, + newpc.pc_sigaction_vec[i].sa_flags);; + failed++; + } + if (oldpc->pc_sigaction_vec[i].sa_sigaction != newpc.pc_sigaction_vec[i].sa_sigaction){ + clicon_log(LOG_WARNING, "%s Plugin %s %s: Changed action of signal %s(%d) from %p to %p", __FUNCTION__, + name, fn, strsignal(i), i, + oldpc->pc_sigaction_vec[i].sa_sigaction, + newpc.pc_sigaction_vec[i].sa_sigaction); + failed++; + } + if (failed){ + oldpc->pc_status = -1; + } + /* assert(oldpc->pc_status == 0); */ + } + retval = 0; + done: + return retval; +} + /*! Call single plugin start callback * @param[in] cp Plugin handle * @param[in] h Clixon handle @@ -497,12 +594,18 @@ clixon_plugin_start_one(clixon_plugin_t *cp, plgstart_t *fn; /* Plugin start */ if ((fn = cp->cp_api.ca_start) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h) < 0) { if (clicon_errno < 0) clicon_log(LOG_WARNING, "%s: Internal error: Start callback in plugin: %s returned -1 but did not make a clicon_err call", __FUNCTION__, cp->cp_name); goto done; } + if (plugin_context_check(&pc, cp->cp_name, __FUNCTION__) < 0) + goto done; } retval = 0; done: @@ -545,12 +648,18 @@ clixon_plugin_exit_one(clixon_plugin_t *cp, plgexit_t *fn; if ((fn = cp->cp_api.ca_exit) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h) < 0) { if (clicon_errno < 0) clicon_log(LOG_WARNING, "%s: Internal error: Exit callback in plugin: %s returned -1 but did not make a clicon_err call", __FUNCTION__, cp->cp_name); goto done; } + if (plugin_context_check(&pc, cp->cp_name, __FUNCTION__) < 0) + goto done; if (dlclose(cp->cp_handle) != 0) { error = (char*)dlerror(); clicon_err(OE_PLUGIN, errno, "dlclose: %s", error ? error : "Unknown error"); @@ -611,12 +720,18 @@ clixon_plugin_auth_one(clixon_plugin_t *cp, clicon_debug(1, "%s", __FUNCTION__); if ((fn = cp->cp_api.ca_auth) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if ((retval = fn(h, req, auth_type, authp)) < 0) { if (clicon_errno < 0) clicon_log(LOG_WARNING, "%s: Internal error: Auth callback in plugin: %s returned -1 but did not make a clicon_err call", __FUNCTION__, cp->cp_name); goto done; } + if (plugin_context_check(&pc, cp->cp_name, __FUNCTION__) < 0) + goto done; } else retval = 0; /* Ignored / no callback */ @@ -688,12 +803,18 @@ clixon_plugin_extension_one(clixon_plugin_t *cp, plgextension_t *fn; /* Plugin extension fn */ if ((fn = cp->cp_api.ca_extension) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h, yext, ys) < 0) { if (clicon_errno < 0) clicon_log(LOG_WARNING, "%s: Internal error: Extension callback in plugin: %s returned -1 but did not make a clicon_err call", __FUNCTION__, cp->cp_name); goto done; } + if (plugin_context_check(&pc, cp->cp_name, __FUNCTION__) < 0) + goto done; } retval = 0; done: @@ -752,12 +873,18 @@ clixon_plugin_datastore_upgrade_one(clixon_plugin_t *cp, datastore_upgrade_t *fn; if ((fn = cp->cp_api.ca_datastore_upgrade) != NULL){ + plugin_context_t pc = {0,}; + + if (plugin_context_get(&pc) < 0) + goto done; if (fn(h, db, xt, msd) < 0) { if (clicon_errno < 0) clicon_log(LOG_WARNING, "%s: Internal error: Datastore upgrade callback in plugin: %s returned -1 but did not make a clicon_err call", __FUNCTION__, cp->cp_name); goto done; } + if (plugin_context_check(&pc, cp->cp_name, __FUNCTION__) < 0) + goto done; } retval = 0; done: