From: Alan T. DeKok Date: Wed, 10 Dec 2025 15:10:32 +0000 (-0500) Subject: update fr_pair_verify() with argument to verify the values X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b6b5e2edd5af714e57be502939838b9a83fd45f2;p=thirdparty%2Ffreeradius-server.git update fr_pair_verify() with argument to verify the values so that we can check more often for invalid values. The default is "true" for the PAIR_VERIFY() and REQUEST_VERIFY() macros. Update the various parsers to pass "false" if they add a VP to a list before setting the value. This lets the tests continue to pass, but also ensures that at normal run-time, we do the full checks. --- diff --git a/src/lib/server/request.c b/src/lib/server/request.c index 3c45dc3a118..192a07ba912 100644 --- a/src/lib/server/request.c +++ b/src/lib/server/request.c @@ -664,11 +664,11 @@ void request_verify(char const *file, int line, request_t const *request) fr_assert(request->magic == REQUEST_MAGIC); (void)talloc_get_type_abort(request->request_ctx, fr_pair_t); - fr_pair_list_verify(file, line, request->request_ctx, &request->request_pairs); + fr_pair_list_verify(file, line, request->request_ctx, &request->request_pairs, true); (void)talloc_get_type_abort(request->reply_ctx, fr_pair_t); - fr_pair_list_verify(file, line, request->reply_ctx, &request->reply_pairs); + fr_pair_list_verify(file, line, request->reply_ctx, &request->reply_pairs, true); (void)talloc_get_type_abort(request->control_ctx, fr_pair_t); - fr_pair_list_verify(file, line, request->control_ctx, &request->control_pairs); + fr_pair_list_verify(file, line, request->control_ctx, &request->control_pairs, true); (void)talloc_get_type_abort(request->session_state_ctx, fr_pair_t); #ifndef NDEBUG @@ -681,8 +681,8 @@ void request_verify(char const *file, int line, request_t const *request) } #endif - fr_pair_list_verify(file, line, request->session_state_ctx, &request->session_state_pairs); - fr_pair_list_verify(file, line, request->local_ctx, &request->local_pairs); + fr_pair_list_verify(file, line, request->session_state_ctx, &request->session_state_pairs, true); + fr_pair_list_verify(file, line, request->local_ctx, &request->local_pairs, true); fr_assert(request->proto_dict != NULL); fr_assert(request->local_dict != NULL); diff --git a/src/lib/server/state.c b/src/lib/server/state.c index 011ad1d65b9..e7857366cf4 100644 --- a/src/lib/server/state.c +++ b/src/lib/server/state.c @@ -755,7 +755,7 @@ int fr_request_to_state(fr_state_tree_t *state, request_t *request) * are parented correctly, else we'll get * memory errors when we restore. */ - fr_pair_list_verify(__FILE__, __LINE__, request->session_state_ctx, &request->session_state_pairs); + fr_pair_list_verify(__FILE__, __LINE__, request->session_state_ctx, &request->session_state_pairs, true); #endif } diff --git a/src/lib/util/pair.c b/src/lib/util/pair.c index e4c375cc40d..bd7cb76274f 100644 --- a/src/lib/util/pair.c +++ b/src/lib/util/pair.c @@ -3090,7 +3090,8 @@ int fr_pair_value_enum_box(fr_value_box_t const **out, fr_pair_t *vp) /* * Verify a fr_pair_t */ -void fr_pair_verify(char const *file, int line, fr_dict_attr_t const *parent_da, fr_pair_list_t const *list, fr_pair_t const *vp) +void fr_pair_verify(char const *file, int line, fr_dict_attr_t const *parent_da, + fr_pair_list_t const *list, fr_pair_t const *vp, bool verify_values) { (void) talloc_get_type_abort_const(vp, fr_pair_t); @@ -3139,14 +3140,12 @@ void fr_pair_verify(char const *file, int line, fr_dict_attr_t const *parent_da, file, line, vp->da->name, vp->da->parent->name, parent->da->name); } -#if 0 /* * We would like to enable this, but there's a * lot of code like fr_pair_append_by_da() which * creates the #fr_pair_t with no value. */ - fr_value_box_verify(file, line, &vp->data); -#endif + if (verify_values) fr_value_box_verify(file, line, &vp->data); } else { fr_pair_t *parent = fr_pair_parent(vp); @@ -3156,7 +3155,7 @@ void fr_pair_verify(char const *file, int line, fr_dict_attr_t const *parent_da, file, line, vp->da->name); } - fr_pair_list_verify(file, line, vp, &vp->vp_group); + fr_pair_list_verify(file, line, vp, &vp->vp_group, verify_values); } switch (vp->vp_type) { @@ -3283,7 +3282,7 @@ void fr_pair_verify(char const *file, int line, fr_dict_attr_t const *parent_da, file, line, child->da->name, child->da->parent->name, vp->da->name); - fr_pair_verify(file, line, vp->da, &vp->vp_group, child); + fr_pair_verify(file, line, vp->da, &vp->vp_group, child, verify_values); } UNCONST(fr_pair_t *, vp)->vp_group.verified = true; @@ -3339,8 +3338,9 @@ void fr_pair_verify(char const *file, int line, fr_dict_attr_t const *parent_da, * @param[in] line number in file * @param[in] expected talloc ctx pairs should have been allocated in * @param[in] list of fr_pair_ts to verify + * @param[in] verify_values whether we verify the values, too. */ -void fr_pair_list_verify(char const *file, int line, TALLOC_CTX const *expected, fr_pair_list_t const *list) +void fr_pair_list_verify(char const *file, int line, TALLOC_CTX const *expected, fr_pair_list_t const *list, bool verify_values) { fr_pair_t *slow, *fast; TALLOC_CTX *parent; @@ -3355,7 +3355,7 @@ void fr_pair_list_verify(char const *file, int line, TALLOC_CTX const *expected, for (slow = fr_pair_list_head(list), fast = fr_pair_list_head(list); slow && fast; slow = fr_pair_list_next(list, slow), fast = fr_pair_list_next(list, fast)) { - PAIR_VERIFY_WITH_LIST(list, slow); + fr_pair_verify(__FILE__, __LINE__, NULL, list, slow, verify_values); /* * Advances twice as fast as slow... @@ -3384,7 +3384,7 @@ void fr_pair_list_verify(char const *file, int line, TALLOC_CTX const *expected, * Check the remaining pairs */ for (; slow; slow = fr_pair_list_next(list, slow)) { - PAIR_VERIFY_WITH_LIST(list, slow); + fr_pair_verify(__FILE__, __LINE__, NULL, list, slow, verify_values); parent = talloc_parent(slow); if (expected && (parent != expected)) goto bad_parent; diff --git a/src/lib/util/pair.h b/src/lib/util/pair.h index c42853ca715..d4b5b49c275 100644 --- a/src/lib/util/pair.h +++ b/src/lib/util/pair.h @@ -158,16 +158,17 @@ struct value_pair_s { * */ #ifdef WITH_VERIFY_PTR -void fr_pair_verify(char const *file, int line, fr_dict_attr_t const *parent, fr_pair_list_t const *list, fr_pair_t const *vp) CC_HINT(nonnull(5)); +void fr_pair_verify(char const *file, int line, fr_dict_attr_t const *parent, + fr_pair_list_t const *list, fr_pair_t const *vp, bool verify_values) CC_HINT(nonnull(5)); void fr_pair_list_verify(char const *file, int line, - TALLOC_CTX const *expected, fr_pair_list_t const *list) CC_HINT(nonnull(4)); + TALLOC_CTX const *expected, fr_pair_list_t const *list, bool verify_values) CC_HINT(nonnull(4)); -# define PAIR_VERIFY(_x) fr_pair_verify(__FILE__, __LINE__, NULL, NULL, _x) -# define PAIR_VERIFY_WITH_LIST(_l, _x) fr_pair_verify(__FILE__, __LINE__, NULL, _l, _x) -# define PAIR_LIST_VERIFY(_x) fr_pair_list_verify(__FILE__, __LINE__, NULL, _x) -# define PAIR_VERIFY_WITH_PARENT_VP(_p, _x) fr_pair_verify(__FILE__, __LINE__, (_p)->da, &(_p)->vp_group, _x) -# define PAIR_VERIFY_WITH_PARENT_DA(_p, _x) fr_pair_verify(__FILE__, __LINE__, _p, NULL, _x) +# define PAIR_VERIFY(_x) fr_pair_verify(__FILE__, __LINE__, NULL, NULL, _x, true) +# define PAIR_VERIFY_WITH_LIST(_l, _x) fr_pair_verify(__FILE__, __LINE__, NULL, _l, _x, true) +# define PAIR_LIST_VERIFY(_x) fr_pair_list_verify(__FILE__, __LINE__, NULL, _x, true) +# define PAIR_VERIFY_WITH_PARENT_VP(_p, _x) fr_pair_verify(__FILE__, __LINE__, (_p)->da, &(_p)->vp_group, _x, true) +# define PAIR_VERIFY_WITH_PARENT_DA(_p, _x) fr_pair_verify(__FILE__, __LINE__, _p, NULL, _x, true) #define PAIR_ALLOCED(_x) do { (_x)->filename = __FILE__; (_x)->line = __LINE__; } while (0) #else diff --git a/src/lib/util/pair_legacy.c b/src/lib/util/pair_legacy.c index eafe41d913a..d3e2f4d2f9c 100644 --- a/src/lib/util/pair_legacy.c +++ b/src/lib/util/pair_legacy.c @@ -326,6 +326,8 @@ redo: slen = fr_dict_oid_component(&err, &da, relative->da, &our_in, &bareword_terminals); if (err == FR_DICT_ATTR_NOTFOUND) { if (raw) { + fr_pair_t *parent_vp; + /* * We looked up raw.FOO, and FOO wasn't found. The component must be a number. */ @@ -357,8 +359,15 @@ redo: if (!vp) return fr_sbuff_error(&our_in); + parent_vp = fr_pair_parent(vp); + fr_assert(parent_vp); + + /* + * @todo - check parent_vp->da, too? But we have to handle the case of + * groups, changing dictionaries, etc. + */ PAIR_ALLOCED(vp); - PAIR_VERIFY(vp); + fr_pair_verify(__FILE__, __LINE__, NULL, &parent_vp->vp_group, vp, false); /* * The above function MAY have jumped ahead a few levels. Ensure @@ -366,10 +375,6 @@ redo: * but only if the parent changed. */ if (relative->da != vp->da->parent) { - fr_pair_t *parent_vp; - - parent_vp = fr_pair_parent(vp); - fr_assert(parent_vp); relative->ctx = parent_vp; relative->da = parent_vp->da; @@ -638,6 +643,9 @@ redo: if (slen <= 0) return fr_sbuff_error(&our_in) + slen; done: + /* + * Now that we have a value, verify the full VP. + */ PAIR_VERIFY(vp); keep_going = false;