diff --git a/CHANGELOG.md b/CHANGELOG.md index 61d19acf..a922c444 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,8 @@ Developers may need to change their code ### Corrected Bugs +* Fixed: SEGV in clixon_netconf_lib functions from internal errors including validation. + * Check xerr argument both before and after call on netconf lib functions * Fixed: RFC 8040 yang-data extension allows non-key lists * Added YANG_FLAG_NOKEY as exception to mandatory key lists * Fixed: mandatory leaf in a uses statement caused abort diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index 6244d200..be7b9377 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -238,7 +238,7 @@ client_get_streams(clicon_handle h, cprintf(cb,"", top); if (clixon_xml_parse_string(cbuf_get(cb), YB_MODULE, yspec, &x, NULL) < 0){ - if (netconf_operation_failed_xml(xret, "protocol", clicon_err_reason)< 0) + if (xret && netconf_operation_failed_xml(xret, "protocol", clicon_err_reason)< 0) goto done; goto fail; } diff --git a/apps/backend/backend_commit.c b/apps/backend/backend_commit.c index 03e318cf..8abcbcbc 100644 --- a/apps/backend/backend_commit.c +++ b/apps/backend/backend_commit.c @@ -132,7 +132,7 @@ generic_validate(clicon_handle h, cprintf(cb, "Mandatory variable of %s in module %s", xml_parent(x1)?xml_name(xml_parent(x1)):"", yang_argument_get(ys_module(ys))); - if (netconf_missing_element_xml(xret, "protocol", xml_name(x1), cbuf_get(cb)) < 0) + if (xret && netconf_missing_element_xml(xret, "protocol", xml_name(x1), cbuf_get(cb)) < 0) goto done; goto fail; } @@ -480,9 +480,9 @@ startup_commit(clicon_handle h, * and call application callback validations. * @param[in] h Clicon handle * @param[in] candidate The candidate database. The wanted backend state - * @param[out] xret Error XML tree. Free with xml_free after use + * @param[out] xret Error XML tree, if retval is 0. Free with xml_free after use * @retval -1 Error - or validation failed (but cbret not set) - * @retval 0 Validation failed (with cbret set) + * @retval 0 Validation failed (with xret set) * @retval 1 Validation OK * @note Need to differentiate between error and validation fail * (only done for generic_validate) @@ -505,16 +505,19 @@ validate_common(clicon_handle h, goto done; } /* This is the state we are going to */ - if (xmldb_get0(h, db, YB_MODULE, NULL, "/", 0, &td->td_target, NULL, NULL) < 0) + if ((ret = xmldb_get0(h, db, YB_MODULE, NULL, "/", 0, &td->td_target, NULL, xret)) < 0) goto done; - + if (ret == 0) + goto fail; /* Clear flags xpath for get */ xml_apply0(td->td_target, CX_ELMNT, (xml_applyfn_t*)xml_flag_reset, (void*)(XML_FLAG_MARK|XML_FLAG_CHANGE)); /* 2. Parse xml trees * This is the state we are going from */ - if (xmldb_get0(h, "running", YB_MODULE, NULL, "/", 0, &td->td_src, NULL, NULL) < 0) + if ((ret = xmldb_get0(h, "running", YB_MODULE, NULL, "/", 0, &td->td_src, NULL, xret)) < 0) goto done; + if (ret == 0) + goto fail; /* Clear flags xpath for get */ xml_apply0(td->td_src, CX_ELMNT, (xml_applyfn_t*)xml_flag_reset, (void*)(XML_FLAG_MARK|XML_FLAG_CHANGE)); @@ -606,11 +609,23 @@ candidate_validate(clicon_handle h, if ((td = transaction_new()) == NULL) goto done; /* Common steps (with commit) */ - if ((ret = validate_common(h, db, td, &xret)) < 1){ + if ((ret = validate_common(h, db, td, &xret)) < 0){ /* A little complex due to several sources of validation fails or errors. * (1) xerr is set -> translate to cbret; (2) cbret set use that; otherwise - * use clicon_err. */ - if (xret && clicon_xml2cbuf(cbret, xret, 0, 0, -1) < 0) + * use clicon_err. + * TODO: -1 return should be fatal error, not failed validation + */ + if (!cbuf_len(cbret) && + netconf_operation_failed(cbret, "application", clicon_err_reason)< 0) + goto done; + goto fail; + } + if (ret == 0){ + if (xret == NULL){ + clicon_err(OE_CFG, EINVAL, "xret is NULL"); + goto done; + } + if (clicon_xml2cbuf(cbret, xret, 0, 0, -1) < 0) goto done; if (!cbuf_len(cbret) && netconf_operation_failed(cbret, "application", clicon_err_reason)< 0) diff --git a/lib/clixon/clixon_netconf_lib.h b/lib/clixon/clixon_netconf_lib.h index f42cf33d..40b9bc96 100644 --- a/lib/clixon/clixon_netconf_lib.h +++ b/lib/clixon/clixon_netconf_lib.h @@ -127,8 +127,7 @@ int netconf_operation_failed(cbuf *cb, char *type, char *message); int netconf_operation_failed_xml(cxobj **xret, char *type, char *message); int netconf_malformed_message(cbuf *cb, char *message); int netconf_malformed_message_xml(cxobj **xret, char *message); -int netconf_data_not_unique_xml(cxobj **xret, cxobj *x, cvec *cvk); - +int netconf_data_not_unique_xml(cxobj **xret, cxobj *x, cvec *cvk); int netconf_minmax_elements_xml(cxobj **xret, cxobj *xp, char *name, int max); int netconf_trymerge(cxobj *x, yang_stmt *yspec, cxobj **xret); int netconf_module_features(clicon_handle h); diff --git a/lib/src/clixon_datastore_read.c b/lib/src/clixon_datastore_read.c index 0413d2c6..c4cf0293 100644 --- a/lib/src/clixon_datastore_read.c +++ b/lib/src/clixon_datastore_read.c @@ -557,7 +557,7 @@ xmldb_readfile(clicon_handle h, } cprintf(cberr, "Internal error: %s", clicon_err_reason); clicon_err_reset(); - if (netconf_operation_failed_xml(xerr, "application", cbuf_get(cberr))< 0) + if (xerr && netconf_operation_failed_xml(xerr, "application", cbuf_get(cberr))< 0) goto done; cbuf_free(cberr); goto fail; diff --git a/lib/src/clixon_json.c b/lib/src/clixon_json.c index b27733b4..caf79351 100644 --- a/lib/src/clixon_json.c +++ b/lib/src/clixon_json.c @@ -1238,7 +1238,7 @@ _json_parse(char *str, goto done; } cprintf(cberr, "Top-level JSON object %s is not qualified with namespace which is a MUST according to RFC 7951", xml_name(x)); - if (netconf_malformed_message_xml(xerr, cbuf_get(cberr)) < 0) + if (xerr && netconf_malformed_message_xml(xerr, cbuf_get(cberr)) < 0) goto done; goto fail; } diff --git a/lib/src/clixon_netconf_lib.c b/lib/src/clixon_netconf_lib.c index febf5b93..8dadfbd5 100644 --- a/lib/src/clixon_netconf_lib.c +++ b/lib/src/clixon_netconf_lib.c @@ -131,6 +131,10 @@ netconf_invalid_value_xml(cxobj **xret, char *encstr = NULL; cxobj *xa; + if (xret == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); + goto done; + } if (*xret == NULL){ if ((*xret = xml_new("rpc-reply", NULL, CX_ELMNT)) == NULL) goto done; @@ -248,6 +252,10 @@ netconf_missing_attribute_xml(cxobj **xret, char *encstr = NULL; cxobj *xa; + if (xret == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); + goto done; + } if (*xret == NULL){ if ((*xret = xml_new("rpc-reply", NULL, CX_ELMNT)) == NULL) goto done; @@ -356,6 +364,10 @@ netconf_bad_attribute_xml(cxobj **xret, char *encstr = NULL; cxobj *xa; + if (xret == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); + goto done; + } if (*xret == NULL){ if ((*xret = xml_new("rpc-reply", NULL, CX_ELMNT)) == NULL) goto done; @@ -449,6 +461,10 @@ netconf_common_xml(cxobj **xret, char *encstr = NULL; cxobj *xa; + if (xret == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); + goto done; + } if (*xret == NULL){ if ((*xret = xml_new("rpc-reply", NULL, CX_ELMNT)) == NULL) goto done; @@ -705,6 +721,10 @@ netconf_access_denied_xml(cxobj **xret, char *encstr = NULL; cxobj *xa; + if (xret == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); + goto done; + } if (*xret == NULL){ if ((*xret = xml_new("rpc-reply", NULL, CX_ELMNT)) == NULL) goto done; @@ -941,6 +961,10 @@ netconf_data_missing_xml(cxobj **xret, cxobj *xerr; cxobj *xa; + if (xret == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); + goto done; + } if (*xret == NULL){ if ((*xret = xml_new("rpc-reply", NULL, CX_ELMNT)) == NULL) goto done; @@ -1005,6 +1029,10 @@ netconf_operation_not_supported_xml(cxobj **xret, char *encstr = NULL; cxobj *xa; + if (xret == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); + goto done; + } if (*xret == NULL){ if ((*xret = xml_new("rpc-reply", NULL, CX_ELMNT)) == NULL) goto done; @@ -1116,6 +1144,10 @@ netconf_operation_failed_xml(cxobj **xret, char *encstr = NULL; cxobj *xa; + if (xret == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); + goto done; + } if (*xret == NULL){ if ((*xret = xml_new("rpc-reply", NULL, CX_ELMNT)) == NULL) goto done; @@ -1200,6 +1232,10 @@ netconf_malformed_message_xml(cxobj **xret, char *encstr = NULL; cxobj *xa; + if (xret == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); + goto done; + } if (*xret == NULL){ if ((*xret = xml_new("rpc-reply", NULL, CX_ELMNT)) == NULL) goto done; @@ -1250,8 +1286,12 @@ netconf_data_not_unique_xml(cxobj **xret, cxobj *xerr; cxobj *xinfo; cbuf *cb = NULL; - cxobj *xa; + cxobj *xa; + if (xret == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); + goto done; + } if (*xret == NULL){ if ((*xret = xml_new("rpc-reply", NULL, CX_ELMNT)) == NULL) goto done; @@ -1316,6 +1356,10 @@ netconf_minmax_elements_xml(cxobj **xret, cbuf *cb = NULL; cxobj *xa; + if (xret == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); + goto done; + } if (*xret == NULL){ if ((*xret = xml_new("rpc-reply", NULL, CX_ELMNT)) == NULL) goto done; @@ -1373,6 +1417,10 @@ netconf_trymerge(cxobj *x, char *reason = NULL; cxobj *xc; + if (xret == NULL){ + clicon_err(OE_NETCONF, EINVAL, "xret is NULL"); + goto done; + } if (*xret == NULL){ if ((*xret = xml_dup(x)) == NULL) goto done; diff --git a/lib/src/clixon_validate.c b/lib/src/clixon_validate.c index 274d339a..802330c8 100644 --- a/lib/src/clixon_validate.c +++ b/lib/src/clixon_validate.c @@ -118,7 +118,7 @@ validate_leafref(cxobj *xt, if ((leafrefbody = xml_body(xt)) == NULL) goto ok; if ((ypath = yang_find(ytype, Y_PATH, NULL)) == NULL){ - if (netconf_missing_element_xml(xret, "application", yang_argument_get(ytype), "Leafref requires path statement") < 0) + if (xret && netconf_missing_element_xml(xret, "application", yang_argument_get(ytype), "Leafref requires path statement") < 0) goto done; goto fail; } @@ -141,7 +141,7 @@ validate_leafref(cxobj *xt, goto done; } cprintf(cberr, "Leafref validation failed: No leaf %s matching path %s", leafrefbody, path); - if (netconf_bad_element_xml(xret, "application", leafrefbody, cbuf_get(cberr)) < 0) + if (xret && netconf_bad_element_xml(xret, "application", leafrefbody, cbuf_get(cberr)) < 0) goto done; goto fail; } @@ -211,7 +211,7 @@ validate_identityref(cxobj *xt, /* Get idref value. Then see if this value is derived from ytype. */ if ((node = xml_body(xt)) == NULL){ /* It may not be empty */ - if (netconf_bad_element_xml(xret, "application", xml_name(xt), "Identityref should not be empty") < 0) + if (xret && netconf_bad_element_xml(xret, "application", xml_name(xt), "Identityref should not be empty") < 0) goto done; goto fail; } @@ -219,13 +219,13 @@ validate_identityref(cxobj *xt, goto done; /* This is the type's base reference */ if ((ybaseref = yang_find(ytype, Y_BASE, NULL)) == NULL){ - if (netconf_missing_element_xml(xret, "application", yang_argument_get(ytype), "Identityref validation failed, no base") < 0) + if (xret && netconf_missing_element_xml(xret, "application", yang_argument_get(ytype), "Identityref validation failed, no base") < 0) goto done; goto fail; } /* This is the actual base identity */ if ((ybaseid = yang_find_identity(ybaseref, yang_argument_get(ybaseref))) == NULL){ - if (netconf_missing_element_xml(xret, "application", yang_argument_get(ybaseref), "Identityref validation failed, no base identity") < 0) + if (xret && netconf_missing_element_xml(xret, "application", yang_argument_get(ybaseref), "Identityref validation failed, no base identity") < 0) goto done; goto fail; } @@ -254,7 +254,7 @@ validate_identityref(cxobj *xt, if (cvec_find(idrefvec, idref) == NULL){ cprintf(cberr, "Identityref validation failed, %s not derived from %s", node, yang_argument_get(ybaseid)); - if (netconf_operation_failed_xml(xret, "application", cbuf_get(cberr)) < 0) + if (xret && netconf_operation_failed_xml(xret, "application", cbuf_get(cberr)) < 0) goto done; goto fail; } @@ -337,7 +337,7 @@ xml_yang_validate_rpc(clicon_handle h, goto done; /* Only accept resolved NETCONF base namespace */ if (namespace == NULL || strcmp(namespace, NETCONF_BASE_NAMESPACE) != 0){ - if (netconf_unknown_namespace_xml(xret, "protocol", rpcprefix, "No appropriate namespace associated with prefix")< 0) + if (xret && netconf_unknown_namespace_xml(xret, "protocol", rpcprefix, "No appropriate namespace associated with prefix")< 0) goto done; goto fail; } @@ -345,7 +345,7 @@ xml_yang_validate_rpc(clicon_handle h, /* xn is name of rpc, ie */ while ((xn = xml_child_each(xrpc, xn, CX_ELMNT)) != NULL) { if ((yn = xml_spec(xn)) == NULL){ - if (netconf_unknown_element_xml(xret, "application", xml_name(xn), NULL) < 0) + if (xret && netconf_unknown_element_xml(xret, "application", xml_name(xn), NULL) < 0) goto done; goto fail; } @@ -434,7 +434,7 @@ check_choice(cxobj *xt, continue; /* not choice */ break; } - if (netconf_bad_element_xml(xret, "application", xml_name(x), "Element in choice statement already exists") < 0) + if (xret && netconf_bad_element_xml(xret, "application", xml_name(x), "Element in choice statement already exists") < 0) goto done; goto fail; } /* while */ @@ -487,7 +487,7 @@ check_list_key(cxobj *xt, while ((cvi = cvec_each(cvk, cvi)) != NULL) { keyname = cv_string_get(cvi); if (xml_find_type(xt, NULL, keyname, CX_ELMNT) == NULL){ - if (netconf_missing_element_xml(xret, "application", keyname, "Mandatory key") < 0) + if (xret && netconf_missing_element_xml(xret, "application", keyname, "Mandatory key") < 0) goto done; goto fail; } @@ -557,7 +557,7 @@ check_mandatory(cxobj *xt, goto done; } cprintf(cb, "Mandatory variable of %s in module %s", xml_name(xt), yang_argument_get(ys_module(yc))); - if (netconf_missing_element_xml(xret, "application", yang_argument_get(yc), cbuf_get(cb)) < 0) + if (xret && netconf_missing_element_xml(xret, "application", yang_argument_get(yc), cbuf_get(cb)) < 0) goto done; goto fail; } @@ -574,7 +574,7 @@ check_mandatory(cxobj *xt, if (x == NULL){ /* @see RFC7950: 15.6 Error Message for Data That Violates * a Mandatory "choice" Statement */ - if (netconf_data_missing_xml(xret, yang_argument_get(yc), NULL) < 0) + if (xret && netconf_data_missing_xml(xret, yang_argument_get(yc), NULL) < 0) goto done; goto fail; } @@ -707,7 +707,7 @@ check_unique_list(cxobj *x, if (cvi==NULL){ /* Last element (i) is newly inserted, see if it is already there */ if (check_insert_duplicate(vec, i, vlen, sorted) < 0){ - if (netconf_data_not_unique_xml(xret, x, cvk) < 0) + if (xret && netconf_data_not_unique_xml(xret, x, cvk) < 0) goto done; goto fail; } @@ -751,7 +751,7 @@ check_min_max(cxobj *xp, if ((ymin = yang_find(y, Y_MIN_ELEMENTS, NULL)) != NULL){ cv = yang_cv_get(ymin); if (nr < cv_uint32_get(cv)){ - if (netconf_minmax_elements_xml(xret, xp, yang_argument_get(y), 0) < 0) + if (xret && netconf_minmax_elements_xml(xret, xp, yang_argument_get(y), 0) < 0) goto done; goto fail; } @@ -760,7 +760,7 @@ check_min_max(cxobj *xp, cv = yang_cv_get(ymax); if (cv_uint32_get(cv) > 0 && /* 0 means unbounded */ nr > cv_uint32_get(cv)){ - if (netconf_minmax_elements_xml(xret, xp, yang_argument_get(y), 1) < 0) + if (xret && netconf_minmax_elements_xml(xret, xp, yang_argument_get(y), 1) < 0) goto done; goto fail; } @@ -851,7 +851,7 @@ check_list_unique_minmax(cxobj *xt, /* Only lists and leaf-lists are allowed to be many * This checks duplicate container and leafs */ - if (netconf_minmax_elements_xml(xret, xt, xml_name(x), 1) < 0) + if (xret && netconf_minmax_elements_xml(xret, xt, xml_name(x), 1) < 0) goto done; goto fail; } @@ -1018,14 +1018,14 @@ xml_yang_validate_add(clicon_handle h, * are considered as "" */ cvtype = cv_type_get(cv); if (cv_isint(cvtype) || cvtype == CGV_BOOL || cvtype == CGV_DEC64){ - if (netconf_bad_element_xml(xret, "application", yang_argument_get(yt), "Invalid NULL value") < 0) + if (xret && netconf_bad_element_xml(xret, "application", yang_argument_get(yt), "Invalid NULL value") < 0) goto done; goto fail; } } else{ if (cv_parse1(body, cv, &reason) != 1){ - if (netconf_bad_element_xml(xret, "application", yang_argument_get(yt), reason) < 0) + if (xret && netconf_bad_element_xml(xret, "application", yang_argument_get(yt), reason) < 0) goto done; goto fail; } @@ -1033,7 +1033,7 @@ xml_yang_validate_add(clicon_handle h, if ((ret = ys_cv_validate(h, cv, yt, NULL, &reason)) < 0) goto done; if (ret == 0){ - if (netconf_bad_element_xml(xret, "application", yang_argument_get(yt), reason) < 0) + if (xret && netconf_bad_element_xml(xret, "application", yang_argument_get(yt), reason) < 0) goto done; goto fail; } @@ -1158,7 +1158,7 @@ xml_yang_validate_all(clicon_handle h, goto done; if (ns) cprintf(cb, " in namespace: %s", ns); - if (netconf_unknown_element_xml(xret, "application", xml_name(xt), cbuf_get(cb)) < 0) + if (xret && netconf_unknown_element_xml(xret, "application", xml_name(xt), cbuf_get(cb)) < 0) goto done; goto fail; } @@ -1217,7 +1217,7 @@ xml_yang_validate_all(clicon_handle h, } cprintf(cb, "Failed MUST xpath '%s' of '%s' in module %s", xpath, xml_name(xt), yang_argument_get(ys_module(ys))); - if (netconf_operation_failed_xml(xret, "application", + if (xret && netconf_operation_failed_xml(xret, "application", ye?yang_argument_get(ye):cbuf_get(cb)) < 0) goto done; goto fail; @@ -1247,7 +1247,7 @@ xml_yang_validate_all(clicon_handle h, cprintf(cb, "Failed WHEN condition of %s in module %s", xml_name(xt), yang_argument_get(ys_module(ys))); - if (netconf_operation_failed_xml(xret, "application", + if (xret && netconf_operation_failed_xml(xret, "application", cbuf_get(cb)) < 0) goto done; goto fail; @@ -1269,7 +1269,7 @@ xml_yang_validate_all(clicon_handle h, xpath, xml_name(xt), yang_argument_get(ys_module(ys))); - if (netconf_operation_failed_xml(xret, "application", + if (xret && netconf_operation_failed_xml(xret, "application", cbuf_get(cb)) < 0) goto done; goto fail; diff --git a/lib/src/clixon_yang_module.c b/lib/src/clixon_yang_module.c index 3d47f6f7..3718ceb8 100644 --- a/lib/src/clixon_yang_module.c +++ b/lib/src/clixon_yang_module.c @@ -329,7 +329,7 @@ yang_modules_state_get(clicon_handle h, * Note, list is not sorted since it is state (should not be) */ if (clixon_xml_parse_string(cbuf_get(cb), YB_MODULE, yspec, &x, NULL) < 0){ - if (netconf_operation_failed_xml(xret, "protocol", clicon_err_reason)< 0) + if (xret && netconf_operation_failed_xml(xret, "protocol", clicon_err_reason)< 0) goto done; goto fail; }