From 4236e0185a1130b777d4616948b357a50c8b0ea0 Mon Sep 17 00:00:00 2001 From: "Alan T. DeKok" Date: Sun, 23 Feb 2025 05:46:51 -0500 Subject: [PATCH] add and use "has_fixup" flag. When we copy an attribute, we need to check if it has pending fixups. If so, we can't copy it. This gives the admin a descriptive error, rather than having something go wrong later. This situation happens when we're cloning an attribute that has children, and those children have fixups. A more in-depth fix would be to move the fixup lists to the fr_dict_t. The cloned attribute could then add itself to a separate "clone after fixups" list. So the clone could be applied last, after all of the fixups have been applied. Part of the fix is checking for pending fixups and complaining. More of the fix is setting "ref=..." immediately, if it can be resolved. That way we avoid many pending fixups. --- src/lib/util/dict.h | 2 ++ src/lib/util/dict_fixup.c | 20 +++++++++++++------- src/lib/util/dict_tokenize.c | 26 ++++++++++++++++++++++---- src/lib/util/dict_util.c | 23 +++++++++++++++++++---- 4 files changed, 56 insertions(+), 15 deletions(-) diff --git a/src/lib/util/dict.h b/src/lib/util/dict.h index 48dec39ed3..0400252ccb 100644 --- a/src/lib/util/dict.h +++ b/src/lib/util/dict.h @@ -114,6 +114,8 @@ typedef struct { unsigned int local : 1; //!< is a local variable + unsigned int has_fixup : 1; + /* * main: extra is set, then this field is is key, bit, or a uint16 length field. * radius: is one of 9 options for flags diff --git a/src/lib/util/dict_fixup.c b/src/lib/util/dict_fixup.c index b96dfda1d2..fb1a846105 100644 --- a/src/lib/util/dict_fixup.c +++ b/src/lib/util/dict_fixup.c @@ -309,10 +309,9 @@ static inline CC_HINT(always_inline) int dict_fixup_enumv_apply(UNUSED dict_fixu ret = fr_dict_enum_add_name(da, fixup->name, &value, false, false); fr_value_box_clear(&value); + da->flags.has_fixup = false; - if (ret < 0) return -1; - - return 0; + return ret; } /** Resolve a group reference @@ -341,6 +340,8 @@ int dict_fixup_group_enqueue(dict_fixup_ctx_t *fctx, fr_dict_attr_t *da, char co .ref = talloc_strdup(fixup, ref), }; + da->flags.has_fixup = true; + return dict_fixup_common(&fctx->group, &fixup->common); } @@ -374,9 +375,10 @@ static inline CC_HINT(always_inline) int dict_fixup_group_apply(UNUSED dict_fixu fr_cwd_strip(fixup->da->filename), fixup->da->line); return -1; } - if (unlikely(dict_attr_ref_resolve(fixup->da, da) < 0)) return -1; - return 0; + fixup->da->flags.has_fixup = false; + + return dict_attr_ref_resolve(fixup->da, da); } /** Clone one area of a tree into another @@ -537,7 +539,7 @@ int dict_fixup_clone(fr_dict_attr_t **dst_p, fr_dict_attr_t const *src) */ if (dict_attr_children(dst)) { if (dict_attr_acopy_children(dict, cloned, dst) < 0) { - fr_strerror_printf("Failed populating attribute '%s' with children of %s", src->name, dst->name); + fr_strerror_printf("Failed populating attribute '%s' with children of %s - %s", src->name, dst->name, fr_strerror()); return -1; } } @@ -547,7 +549,7 @@ int dict_fixup_clone(fr_dict_attr_t **dst_p, fr_dict_attr_t const *src) */ if (dict_attr_children(src)) { if (dict_attr_acopy_children(dict, cloned, src) < 0) { - fr_strerror_printf("Failed populating attribute '%s' with children of %s", src->name, dst->name); + fr_strerror_printf("Failed populating attribute '%s' with children of %s - %s", src->name, dst->name, fr_strerror()); return -1; } } @@ -585,6 +587,7 @@ static inline CC_HINT(always_inline) int dict_fixup_clone_apply(UNUSED dict_fixu return -1; } + fixup->da->flags.has_fixup = false; return dict_fixup_clone(&fixup->da, src); } @@ -685,6 +688,7 @@ static inline CC_HINT(always_inline) int dict_fixup_clone_enum_apply(UNUSED dict return -1; } + fixup->da->flags.has_fixup = false; return 0; } @@ -742,6 +746,7 @@ static inline CC_HINT(always_inline) int dict_fixup_vsa_apply(UNUSED dict_fixup_ if (fr_dict_attr_add(dict, fixup->da, dv->name, dv->pen, FR_TYPE_VENDOR, NULL) < 0) return -1; } + fixup->da->flags.has_fixup = false; return 0; } @@ -802,6 +807,7 @@ static inline CC_HINT(always_inline) int dict_fixup_alias_apply(UNUSED dict_fixu return -1; } + fr_dict_attr_unconst(da)->flags.has_fixup = false; return dict_attr_alias_add(fixup->alias_parent, fixup->alias, da); } diff --git a/src/lib/util/dict_tokenize.c b/src/lib/util/dict_tokenize.c index 702650d7ad..cb2abefc81 100644 --- a/src/lib/util/dict_tokenize.c +++ b/src/lib/util/dict_tokenize.c @@ -840,6 +840,11 @@ static int dict_attr_add_or_fixup(dict_fixup_ctx_t *fixup, fr_dict_attr_t **da_p */ ref = fr_dict_attr_ext(*da_p, FR_DICT_ATTR_EXT_REF); if (ref && fr_dict_attr_ref_is_unresolved(ref->type)) { + /* + * See if we can immediately apply the ref. + */ + fr_dict_attr_t const *src; + switch (fr_dict_attr_ref_type(ref->type)) { case FR_DICT_ATTR_REF_ALIAS: if (fr_dict_attr_add_initialised(da) < 0) { @@ -849,21 +854,35 @@ static int dict_attr_add_or_fixup(dict_fixup_ctx_t *fixup, fr_dict_attr_t **da_p return -1; } + /* + * IF the ref exists, we can always add it. The ref won't be changed later. + */ + src = dict_protocol_reference(da->parent, ref->unresolved, true); + if (src) { + if (dict_attr_ref_set(*da_p, src, FR_DICT_ATTR_REF_ALIAS) < 0) return -1; + break; + } + if (dict_fixup_group_enqueue(fixup, da, ref->unresolved) < 0) return -1; + ret = 1; break; case FR_DICT_ATTR_REF_ENUM: + /* + * Do NOT copy the enums now. Later dictionaries may add more values, and we + * want to be able to copy all values. + */ if (fr_dict_attr_add_initialised(da) < 0) goto error; if (dict_fixup_clone_enum_enqueue(fixup, da, ref->unresolved) < 0) return -1; break; case FR_DICT_ATTR_REF_CLONE: - { /* - * See if we can immediately apply the clone + * @todo - if we defer this clone, we get errors loading dictionary.wimax. That + * likely means there are issues with the dict_fixup_clone_apply() function. */ - fr_dict_attr_t const *src = dict_protocol_reference(da->parent, ref->unresolved, true); + src = dict_protocol_reference(da->parent, ref->unresolved, true); if (src) { if (dict_fixup_clone(da_p, src) < 0) return -1; break; @@ -871,7 +890,6 @@ static int dict_attr_add_or_fixup(dict_fixup_ctx_t *fixup, fr_dict_attr_t **da_p if (dict_fixup_clone_enqueue(fixup, da, ref->unresolved) < 0) return -1; ret = 1; - } break; default: diff --git a/src/lib/util/dict_util.c b/src/lib/util/dict_util.c index 0c2a49d3a3..8d117a53d0 100644 --- a/src/lib/util/dict_util.c +++ b/src/lib/util/dict_util.c @@ -1022,6 +1022,12 @@ fr_dict_attr_t *dict_attr_acopy(TALLOC_CTX *ctx, fr_dict_attr_t const *in, char { fr_dict_attr_t *n; + if (in->flags.has_fixup) { + fr_strerror_printf("Cannot copy from %s - source attribute is waiting for additional definitions", + in->name); + return NULL; + } + n = dict_attr_alloc(ctx, in->parent, new_name ? new_name : in->name, in->attr, in->type, &(dict_attr_args_t){ .flags = &in->flags }); if (unlikely(!n)) return NULL; @@ -1048,6 +1054,12 @@ static fr_dict_attr_t *dict_attr_acopy_dict(TALLOC_CTX *ctx, fr_dict_attr_t *par { fr_dict_attr_t *n; + if (in->flags.has_fixup) { + fr_strerror_printf("Cannot copy from %s - source attribute is waiting for additional definitions", + in->name); + return NULL; + } + n = dict_attr_alloc(ctx, parent, in->name, in->attr, in->type, &(dict_attr_args_t){ .flags = &in->flags }); if (unlikely(!n)) return NULL; @@ -1068,6 +1080,12 @@ int fr_dict_attr_acopy_local(fr_dict_attr_t const *dst, fr_dict_attr_t const *sr return -1; } + if (src->flags.has_fixup) { + fr_strerror_printf("Cannot copy from %s to %s - source attribute is waiting for additional definitions", + src->name, dst->name); + return -1; + } + /* * Why not? @todo - check and fix */ @@ -1106,10 +1124,7 @@ int dict_attr_acopy_children(fr_dict_t *dict, fr_dict_attr_t *dst, fr_dict_attr_ } else { copy = dict_attr_acopy_dict(dict->pool, dst, child); } - if (!copy) { - fr_strerror_printf("Failed cloning child %s", child->name); - return -1; - } + if (!copy) return -1; copy->parent = dst; copy->depth += depth_diff; -- 2.47.3