From 46137e1394c1a6d90afcca01c3c617458a12bcdc Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Wed, 18 Jan 2023 15:55:49 +0100 Subject: [PATCH] Fixed: [Netconf commit confirm session-id mismatch #407](https://github.com/clicon/clixon/issues/407) --- CHANGELOG.md | 1 + apps/backend/backend_client.c | 2 +- apps/backend/backend_commit.c | 8 ++++-- apps/backend/backend_confirm.c | 43 ++++++++++++++-------------- apps/backend/clixon_backend_commit.h | 4 +-- lib/src/clixon_data.c | 4 +++ util/clixon_util_validate.c | 2 +- 7 files changed, 36 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21ae2e01..20703d0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,6 +93,7 @@ Developers may need to change their code ### Corrected Bugs +* Fixed: [Netconf commit confirm session-id mismatch #407](https://github.com/clicon/clixon/issues/407) * Fixed: Initialized session-id to 1 instead of 0 following ietf-netconf.yang * Fixed: [snmpwalk doesn't show properly SNMP boolean values which equal false](https://github.com/clicon/clixon/issues/400) * Fixed: yang-library: Remove revision if empty instead of sending empty revision diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index 0c3bdf50..03f52b63 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -567,7 +567,7 @@ from_client_edit_config(clicon_handle h, break; } } - if ((ret = candidate_commit(h, NULL, "candidate", cbret)) < 0){ /* Assume validation fail, nofatal */ + if ((ret = candidate_commit(h, NULL, "candidate", myid, cbret)) < 0){ /* Assume validation fail, nofatal */ if (netconf_operation_failed(cbret, "application", clicon_err_reason)< 0) goto done; xmldb_copy(h, "running", "candidate"); diff --git a/apps/backend/backend_commit.c b/apps/backend/backend_commit.c index ffa49031..ea4082e1 100644 --- a/apps/backend/backend_commit.c +++ b/apps/backend/backend_commit.c @@ -648,6 +648,7 @@ candidate_validate(clicon_handle h, * @param[in] h Clicon handle * @param[in] xe Request: (or NULL) * @param[in] db A candidate database, not necessarily "candidate" + * @param[in] myid Client id of triggering incoming message (or 0) * @param[out] cbret Return xml tree, eg ..., value that matches the value accompanying the prior confirmed-commit * * @param[in] h Clicon handle - * @param[in] xe Request: + * @param[in] xe Request: * @param[in] myid current client session-id * @retval 1 The confirming-commit is valid * @retval 0 The confirming-commit is not valid @@ -419,6 +419,8 @@ check_valid_confirming_commit(clicon_handle h, clicon_err(OE_CFG, EINVAL, "xe is NULL"); goto done; } + if (myid == 0) + goto invalid; switch (confirmed_commit_state_get(h)) { case PERSISTENT: if (xe_persist_id(xe, &persist_id)) { @@ -470,15 +472,18 @@ check_valid_confirming_commit(clicon_handle h, * * @param[in] h Clicon handle * @param[in] xe Commit rpc xml or NULL + * @param[in] myid Current session-id, only valid > 0 if call is made as a result of an incoming message * @retval 0 OK * @retval -1 Error + * @note There are some calls to this function where myid is 0 (which is invalid). It is unclear if such calls + * actually occur, and if so, if they are correctly handled. The calls are from do_rollback() and load_failsafe() */ int handle_confirmed_commit(clicon_handle h, - cxobj *xe) + cxobj *xe, + uint32_t myid) { int retval = -1; - uint32_t session_id; char *persist; unsigned long confirm_timeout = 0L; int cc_valid; @@ -488,17 +493,14 @@ handle_confirmed_commit(clicon_handle h, clicon_err(OE_CFG, EINVAL, "xe is NULL"); goto done; } - if (clicon_session_id_get(h, &session_id) < 0) { - clicon_err(OE_DAEMON, 0, - "an ephemeral confirmed-commit was issued, but the session-id could not be determined"); - goto done; - }; + if (myid == 0) + goto ok; /* The case of a valid confirming-commit is also handled in the first phase, but only if there is no subsequent * confirmed-commit. It is tested again here as the case of a valid confirming-commit *with* a subsequent * confirmed-commit must be handled once the transaction has begun and after all the plugins' validate callbacks * have been called. */ - cc_valid = check_valid_confirming_commit(h, xe, session_id); + cc_valid = check_valid_confirming_commit(h, xe, myid); if (cc_valid) { if (cancel_rollback_event(h) < 0) { clicon_err(OE_DAEMON, 0, "A valid confirming-commit was received, but the corresponding rollback event was not found"); @@ -538,7 +540,7 @@ handle_confirmed_commit(clicon_handle h, /* The client did not pass a value for and therefore any subsequent confirming-commit must be * issued within the same session. */ - confirmed_commit_session_id_set(h, session_id); + confirmed_commit_session_id_set(h, myid); confirmed_commit_state_set(h, EPHEMERAL); clicon_log(LOG_INFO, @@ -603,6 +605,7 @@ handle_confirmed_commit(clicon_handle h, goto done; } } + ok: retval = 0; done: return retval; @@ -650,7 +653,7 @@ do_rollback(clicon_handle h, confirmed_commit_persist_id_set(h, NULL); } confirmed_commit_state_set(h, ROLLBACK); - if (candidate_commit(h, NULL, "rollback", cbret) < 0) { /* Assume validation fail, nofatal */ + if (candidate_commit(h, NULL, "rollback", 0, cbret) < 0) { /* Assume validation fail, nofatal */ /* theoretically, this should never error, since the rollback database was previously active and therefore * had itself been previously and successfully committed. */ @@ -713,12 +716,14 @@ from_client_cancel_commit(clicon_handle h, void *arg, void *regarg) { - cxobj *persist_id_xml; - char *persist_id = NULL; - uint32_t cur_session_id; - int retval = -1; - int rollback = 0; + cxobj *persist_id_xml; + char *persist_id = NULL; + uint32_t myid; + int retval = -1; + int rollback = 0; + struct client_entry *ce = (struct client_entry *)arg; + myid = ce->ce_id; if ((persist_id_xml = xml_find_type(xe, NULL, "persist-id", CX_ELMNT)) != NULL) { /* persist == persist_id == NULL is legal */ persist_id = xml_body(persist_id_xml); @@ -729,11 +734,7 @@ from_client_cancel_commit(clicon_handle h, if (netconf_invalid_value(cbret, "protocol", "current confirmed-commit is not persistent") < 0) goto done; } - else if (clicon_session_id_get(h, &cur_session_id) < 0) { - if (netconf_invalid_value(cbret, "application", "session-id was not set") < 0) - goto done; - } - else if (cur_session_id != confirmed_commit_session_id_get(h)) { + else if (myid != confirmed_commit_session_id_get(h)) { if (netconf_invalid_value(cbret, "protocol", "confirming-commit must be given within session that gave the confirmed-commit") < 0) goto done; } diff --git a/apps/backend/clixon_backend_commit.h b/apps/backend/clixon_backend_commit.h index 666f7562..808298d5 100644 --- a/apps/backend/clixon_backend_commit.h +++ b/apps/backend/clixon_backend_commit.h @@ -62,7 +62,7 @@ enum confirmed_commit_state confirmed_commit_state_get(clicon_handle h); uint32_t confirmed_commit_session_id_get(clicon_handle h); int cancel_rollback_event(clicon_handle h); int cancel_confirmed_commit(clicon_handle h); -int handle_confirmed_commit(clicon_handle h, cxobj *xe); +int handle_confirmed_commit(clicon_handle h, cxobj *xe, uint32_t myid); int do_rollback(clicon_handle h, uint8_t *errs); int from_client_cancel_commit(clicon_handle h, cxobj *xe, cbuf *cbret, void *arg, void *regarg); int from_client_confirmed_commit(clicon_handle h, cxobj *xe, uint32_t myid, cbuf *cbret); @@ -71,7 +71,7 @@ int from_client_confirmed_commit(clicon_handle h, cxobj *xe, uint32_t myid, cbuf int startup_validate(clicon_handle h, char *db, cxobj **xtr, cbuf *cbret); int startup_commit(clicon_handle h, char *db, cbuf *cbret); int candidate_validate(clicon_handle h, char *db, cbuf *cbret); -int candidate_commit(clicon_handle h, cxobj *xe, char *db, cbuf *cbret); +int candidate_commit(clicon_handle h, cxobj *xe, char *db, uint32_t myid, cbuf *cbret); int from_client_commit(clicon_handle h, cxobj *xe, cbuf *cbret, void *arg, void *regarg); int from_client_discard_changes(clicon_handle h, cxobj *xe, cbuf *cbret, void *arg, void *regarg); diff --git a/lib/src/clixon_data.c b/lib/src/clixon_data.c index daaca13c..8e3c0916 100644 --- a/lib/src/clixon_data.c +++ b/lib/src/clixon_data.c @@ -808,12 +808,16 @@ clicon_db_elmnt_set(clicon_handle h, } /*! Get session id + * * @param[in] h Clicon handle * @param[out] sid Session identifier * @retval 0 OK * @retval -1 Session id not set * Session-ids survive TCP sessions that are created for each message sent to the backend. * The backend assigns session-id for clients: backend assigns, clients get it from backend. + * @note a client will get the currrent session-id of that client, BUT + * a backend will get the next session-id to be assigned + * A backend getting a session-id of an ongoing session should use ce->ce_id */ int clicon_session_id_get(clicon_handle h, diff --git a/util/clixon_util_validate.c b/util/clixon_util_validate.c index 906d7a2b..a9a3550a 100644 --- a/util/clixon_util_validate.c +++ b/util/clixon_util_validate.c @@ -227,7 +227,7 @@ main(int argc, goto done; } if (commit){ - if ((ret = candidate_commit(h, NULL, database, cb)) < 0) + if ((ret = candidate_commit(h, NULL, database, 0, cb)) < 0) goto done; } else{