From: Arran Cudbard-Bell Date: Mon, 11 Nov 2024 22:40:33 +0000 (-0600) Subject: Be _EXPLICIT_ about when we want to refer to attributes primarily by name X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2565977cc23a4e6d9ab564b4c57a045758394f47;p=thirdparty%2Ffreeradius-server.git Be _EXPLICIT_ about when we want to refer to attributes primarily by name --- diff --git a/src/lib/server/virtual_servers.c b/src/lib/server/virtual_servers.c index a3598e0e5cb..254697a9bef 100644 --- a/src/lib/server/virtual_servers.c +++ b/src/lib/server/virtual_servers.c @@ -1340,7 +1340,7 @@ static int define_server_attrs(CONF_SECTION *cs, fr_dict_t *dict, fr_dict_attr_t return -1; } - if (fr_dict_attr_add(dict, parent, value, 0, type, &flags) < 0) { + if (fr_dict_attr_add_name_only(dict, parent, value, type, &flags) < 0) { cf_log_err(ci, "Failed adding local variable '%s' - %s", value, fr_strerror()); return -1; } diff --git a/src/lib/util/dict.h b/src/lib/util/dict.h index e486df87ee0..c6e9e2ab6a1 100644 --- a/src/lib/util/dict.h +++ b/src/lib/util/dict.h @@ -95,7 +95,10 @@ typedef struct { unsigned int counter : 1; //!< integer attribute is actually an impulse / counter - unsigned int name_only : 1; //!< this attribute should always be referred to by name, not by number + unsigned int name_only : 1; //!< this attribute should always be referred to by name. + ///< A number will be allocated, but the allocation scheme + ///< will depend on the parent, and definition type, and + ///< may not be stable in all instances. unsigned int secret : 1; //!< this attribute should be omitted in debug mode @@ -188,6 +191,11 @@ struct dict_attr_s { fr_dict_attr_flags_t flags; //!< Flags. + bool attr_set:1; //!< Attribute number has been set. + //!< We need the full range of values 0-UINT32_MAX + ///< so we can't use any attr values to indicate + ///< "unsetness". + char const *filename; //!< Where the attribute was defined. ///< this buffer's lifetime is bound to the ///< fr_dict_t. @@ -503,6 +511,9 @@ int fr_dict_attr_add_initialised(fr_dict_attr_t *da) CC_HINT(nonnull); int fr_dict_attr_add(fr_dict_t *dict, fr_dict_attr_t const *parent, char const *name, unsigned int attr, fr_type_t type, fr_dict_attr_flags_t const *flags) CC_HINT(nonnull(1,2,3)); +int fr_dict_attr_add_name_only(fr_dict_t *dict, fr_dict_attr_t const *parent, + char const *name, fr_type_t type, fr_dict_attr_flags_t const *flags) CC_HINT(nonnull(1,2,3)); + int fr_dict_enum_add_name(fr_dict_attr_t *da, char const *name, fr_value_box_t const *value, bool coerce, bool replace); diff --git a/src/lib/util/dict_fixup.c b/src/lib/util/dict_fixup.c index 16233d37790..9c28f3d6eab 100644 --- a/src/lib/util/dict_fixup.c +++ b/src/lib/util/dict_fixup.c @@ -728,8 +728,7 @@ static inline CC_HINT(always_inline) int dict_fixup_vsa_apply(UNUSED dict_fixup_ dv = fr_hash_table_iter_next(dict->vendors_by_num, &iter)) { if (dict_attr_child_by_num(fixup->da, dv->pen)) continue; - if (fr_dict_attr_add(dict, fixup->da, dv->name, dv->pen, - FR_TYPE_VENDOR, &(fr_dict_attr_flags_t) {}) < 0) return -1; + if (fr_dict_attr_add(dict, fixup->da, dv->name, dv->pen, FR_TYPE_VENDOR, NULL) < 0) return -1; } return 0; diff --git a/src/lib/util/dict_priv.h b/src/lib/util/dict_priv.h index 0312b9f5424..0136ed64963 100644 --- a/src/lib/util/dict_priv.h +++ b/src/lib/util/dict_priv.h @@ -193,6 +193,8 @@ int dict_attr_parent_init(fr_dict_attr_t **da_p, fr_dict_attr_t const *parent) int dict_attr_num_init(fr_dict_attr_t *da, unsigned int num); +int dict_attr_num_init_name_only(fr_dict_attr_t *da); + void dict_attr_location_init(fr_dict_attr_t *da, char const *filename, int line); int dict_attr_finalise(fr_dict_attr_t **da_p, char const *name); @@ -207,15 +209,23 @@ int dict_attr_finalise(fr_dict_attr_t **da_p, char const *name); * @{ */ #define dict_attr_init(_da_p, _parent, _name, _attr, _type, _args) \ - _dict_attr_init(__FILE__, __LINE__, _da_p, _parent, _name, _attr, _type, _args) + _dict_attr_init(__FILE__, __LINE__, _da_p, _parent, _name, _attr, _type, _args) int _dict_attr_init(char const *filename, int line, fr_dict_attr_t **da_p, fr_dict_attr_t const *parent, char const *name, unsigned int attr, fr_type_t type, dict_attr_args_t const *args) CC_HINT(nonnull(1)); +#define dict_attr_init_name_only(_da_p, _parent, _name, _type, _args) \ + _dict_attr_init_name_only(__FILE__, __LINE__, _da_p, _parent, _name, _type, _args) + +int _dict_attr_init_name_only(char const *filename, int line, + fr_dict_attr_t **da_p, fr_dict_attr_t const *parent, + char const *name, + fr_type_t type, dict_attr_args_t const *args) CC_HINT(nonnull(1)); + #define dict_attr_alloc_root(_ctx, _dict, _name, _attr, _args) \ - _dict_attr_alloc_root(__FILE__, __LINE__, _ctx, _dict, _name, _attr, _args) + _dict_attr_alloc_root(__FILE__, __LINE__, _ctx, _dict, _name, _attr, _args) fr_dict_attr_t *_dict_attr_alloc_root(char const *filename, int line, TALLOC_CTX *ctx, fr_dict_t const *dict, diff --git a/src/lib/util/dict_util.c b/src/lib/util/dict_util.c index ffb4dc9c7e1..cc3551b2c3d 100644 --- a/src/lib/util/dict_util.c +++ b/src/lib/util/dict_util.c @@ -643,15 +643,31 @@ int dict_attr_parent_init(fr_dict_attr_t **da_p, fr_dict_attr_t const *parent) */ int dict_attr_num_init(fr_dict_attr_t *da, unsigned int num) { - if (da->attr != 0) { + if (da->attr_set) { fr_strerror_const("Attribute number already set"); return -1; } da->attr = num; + da->attr_set = true; return 0; } +/** Set the attribute number (if any) + * + * @note Must have a parent set. + * + * @param[in] da to set the attribute number for. + */ +int dict_attr_num_init_name_only(fr_dict_attr_t *da) +{ + if (!da->parent) { + fr_strerror_const("Attribute must have parent set before automatically setting attribute number"); + return -1; + } + return dict_attr_num_init(da, ++fr_dict_attr_unconst(da->parent)->last_child_attr); +} + /** Set where the dictionary attribute was defined * */ @@ -737,6 +753,25 @@ int dict_attr_finalise(fr_dict_attr_t **da_p, char const *name) return 0; } +static inline CC_HINT(always_inline) +int dict_attr_init_common(char const *filename, int line, + fr_dict_attr_t **da_p, + fr_dict_attr_t const *parent, + fr_type_t type, dict_attr_args_t const *args) +{ + dict_attr_location_init((*da_p), filename, line); + + if (unlikely(dict_attr_type_init(da_p, type) < 0)) return -1; + + if (parent && (dict_attr_parent_init(da_p, parent) < 0)) return -1;; + + if (args->ref && (dict_attr_ref_aset(da_p, args->ref, FR_DICT_ATTR_REF_ALIAS) < 0)) return -1; + + if (args->flags) (*da_p)->flags = *args->flags; + + return 0; +} + /** Initialise fields in a dictionary attribute structure * * This function is a wrapper around the other initialisation functions. @@ -769,17 +804,53 @@ int _dict_attr_init(char const *filename, int line, char const *name, unsigned int attr, fr_type_t type, dict_attr_args_t const *args) { - dict_attr_location_init((*da_p), filename, line); + if (unlikely(dict_attr_init_common(filename, line, da_p, parent, type, args) < 0)) return -1; - if (unlikely(dict_attr_type_init(da_p, type) < 0)) return -1; + if (unlikely(dict_attr_num_init(*da_p, attr) < 0)) return -1; - if (parent && (dict_attr_parent_init(da_p, parent) < 0)) return -1;; + if (unlikely(dict_attr_finalise(da_p, name) < 0)) return -1; - if (unlikely(dict_attr_num_init(*da_p, attr) < 0)) return -1; + return 0; +} - if (args->ref && (dict_attr_ref_aset(da_p, args->ref, FR_DICT_ATTR_REF_ALIAS) < 0)) return -1; +/** Initialise fields in a dictionary attribute structure + * + * This function is a wrapper around the other initialisation functions. + * + * The reason for the separation, is that sometimes we're initialising a dictionary attribute + * by parsing an actual dictionary file, and other times we're copying attribute, or initialising + * them programatically. + * + * This function should only be used for the second case, where we have a complet attribute + * definition already. + * + * @note This function can only be used _before_ the attribute is inserted into the dictionary. + * + * @param[in] filename file. + * @param[in] line number. + * @param[in] da_p to initialise. + * @param[in] parent of the attribute, if none, this attribute will + * be initialised as a dictionary root. + * @param[in] name of attribute. Pass NULL for auto-generated name. + * automatically generated. + * @param[in] type of the attribute. + * @param[in] args optional initialisation arguments. + * @return + * - 0 on success. + * - <0 on error. + */ +int _dict_attr_init_name_only(char const *filename, int line, + fr_dict_attr_t **da_p, + fr_dict_attr_t const *parent, + char const *name, + fr_type_t type, dict_attr_args_t const *args) +{ + if (unlikely(dict_attr_init_common(filename, line, da_p, parent, type, args) < 0)) return -1; - if (args->flags) (*da_p)->flags = *args->flags; + /* + * Automatically generate the attribute number when the attribut is added. + */ + (*da_p)->flags.name_only = true; if (unlikely(dict_attr_finalise(da_p, name) < 0)) return -1; @@ -1525,6 +1596,32 @@ int fr_dict_attr_add_initialised(fr_dict_attr_t *da) return -1; } + /* + * In some cases name_only attributes may have explicitly + * assigned numbers. Ensure that there are no conflicts + * between auto-assigned and explkicitly assigned. + */ + if (da->flags.name_only) { + if (da->attr_set) { + fr_dict_attr_t *parent = fr_dict_attr_unconst(da->parent); + + if (da->attr > da->parent->last_child_attr) { + parent->last_child_attr = da->attr; + + /* + * If the attribute is outside of the bounds of + * the type size, then it MUST be an internal + * attribute. Set the flag in this attribute, so + * that the encoder doesn't have to do complex + * checks. + */ + if ((da->attr >= (((uint64_t)1) << (8 * parent->flags.type_size)))) da->flags.internal = true; + } + } else if (unlikely(dict_attr_num_init_name_only(da)) < 0) { + return -1; + } + } + /* * Attributes can also be indexed by number. Ensure that * all attributes of the same number have the same @@ -1538,30 +1635,6 @@ int fr_dict_attr_add_initialised(fr_dict_attr_t *da) return -1; } - /* - * In some cases name_only attributes may have explicitly - * assigned numbers. Ensure that there are no conflicts - * between auto-assigned and explkicitly assigned. - */ - if (da->flags.name_only) { - fr_dict_attr_t *parent = fr_dict_attr_unconst(da->parent); - - if (da->attr > da->parent->last_child_attr) { - parent->last_child_attr = da->attr; - - /* - * If the attribute is outside of the bounds of - * the type size, then it MUST be an internal - * attribute. Set the flag in this attribute, so - * that the encoder doesn't have to do complex - * checks. - */ - if ((da->attr >= (((uint64_t)1) << (8 * parent->flags.type_size)))) da->flags.internal = true; - } else { - if (unlikely(dict_attr_num_init(da, ++fr_dict_attr_unconst(da->parent)->last_child_attr)) < 0) return -1; - } - } - /* * Add in by number */ @@ -1622,6 +1695,32 @@ int fr_dict_attr_add(fr_dict_t *dict, fr_dict_attr_t const *parent, return fr_dict_attr_add_initialised(da); } +/** Add an attribute to the dictionary + * + * @param[in] dict of protocol context we're operating in. + * If NULL the internal dictionary will be used. + * @param[in] parent to add attribute under. + * @param[in] name of the attribute. + * @param[in] type of attribute. + * @param[in] flags to set in the attribute. + * @return + * - 0 on success. + * - -1 on failure. + */ +int fr_dict_attr_add_name_only(fr_dict_t *dict, fr_dict_attr_t const *parent, + char const *name, fr_type_t type, fr_dict_attr_flags_t const *flags) +{ + fr_dict_attr_t *da; + + da = dict_attr_alloc_null(dict->pool, dict->proto); + if (unlikely(!da)) return -1; + + if (dict_attr_init_name_only(&da, parent, name,type, &(dict_attr_args_t){ .flags = flags}) < 0) return -1; + + return fr_dict_attr_add_initialised(da); +} + + int dict_attr_enum_add_name(fr_dict_attr_t *da, char const *name, fr_value_box_t const *value, bool coerce, bool takes_precedence, diff --git a/src/modules/rlm_ldap/rlm_ldap.c b/src/modules/rlm_ldap/rlm_ldap.c index 6a772bfc7b9..f4189ab2336 100644 --- a/src/modules/rlm_ldap/rlm_ldap.c +++ b/src/modules/rlm_ldap/rlm_ldap.c @@ -2673,10 +2673,8 @@ static int mod_bootstrap(module_inst_ctx_t const *mctx) * If the group attribute was not in the dictionary, create it */ if (!boot->group_da) { - fr_dict_attr_flags_t flags = { .name_only = 1 }; - - if (fr_dict_attr_add(fr_dict_unconst(dict_freeradius), fr_dict_root(dict_freeradius), - group_attribute, 0, FR_TYPE_STRING, &flags) < 0) { + if (fr_dict_attr_add_name_only(fr_dict_unconst(dict_freeradius), fr_dict_root(dict_freeradius), + group_attribute, FR_TYPE_STRING, NULL) < 0) { PERROR("Error creating group attribute"); return -1; @@ -2691,10 +2689,8 @@ static int mod_bootstrap(module_inst_ctx_t const *mctx) if (inst->group.cache_attribute) { boot->cache_da = fr_dict_attr_by_name(NULL, fr_dict_root(dict_freeradius), inst->group.cache_attribute); if (!boot->cache_da) { - fr_dict_attr_flags_t flags = { .name_only = 1 }; - - if (fr_dict_attr_add(fr_dict_unconst(dict_freeradius), fr_dict_root(dict_freeradius), - inst->group.cache_attribute, 0, FR_TYPE_STRING, &flags) < 0) { + if (fr_dict_attr_add_name_only(fr_dict_unconst(dict_freeradius), fr_dict_root(dict_freeradius), + inst->group.cache_attribute, FR_TYPE_STRING, NULL) < 0) { PERROR("Error creating cache attribute"); return -1; } diff --git a/src/modules/rlm_sql/rlm_sql.c b/src/modules/rlm_sql/rlm_sql.c index 243e79497f8..f4dd4cc9d9f 100644 --- a/src/modules/rlm_sql/rlm_sql.c +++ b/src/modules/rlm_sql/rlm_sql.c @@ -2213,7 +2213,6 @@ static int mod_bootstrap(module_inst_ctx_t const *mctx) */ if (cf_pair_find(conf, "group_membership_query")) { char const *group_attribute; - fr_dict_attr_flags_t flags = { .name_only = 1 }; char buffer[256]; if (inst->config.group_attribute) { @@ -2227,8 +2226,7 @@ static int mod_bootstrap(module_inst_ctx_t const *mctx) boot->group_da = fr_dict_attr_by_name(NULL, fr_dict_root(dict_freeradius), group_attribute); if (!boot->group_da) { - if (fr_dict_attr_add(fr_dict_unconst(dict_freeradius), fr_dict_root(dict_freeradius), group_attribute, 0, - FR_TYPE_STRING, &flags) < 0) { + if (fr_dict_attr_add_name_only(fr_dict_unconst(dict_freeradius), fr_dict_root(dict_freeradius), group_attribute, FR_TYPE_STRING, NULL) < 0) { cf_log_perr(conf, "Failed defining group attribute"); return -1; } diff --git a/src/modules/rlm_sqlcounter/rlm_sqlcounter.c b/src/modules/rlm_sqlcounter/rlm_sqlcounter.c index a712f718b61..c0f1a6590cd 100644 --- a/src/modules/rlm_sqlcounter/rlm_sqlcounter.c +++ b/src/modules/rlm_sqlcounter/rlm_sqlcounter.c @@ -542,8 +542,8 @@ static int mod_instantiate(module_inst_ctx_t const *mctx) static inline int attr_check(CONF_SECTION *conf, tmpl_t *tmpl, char const *name, fr_dict_attr_flags_t *flags) { if (tmpl_is_attr_unresolved(tmpl) && !fr_dict_attr_by_name(NULL, fr_dict_root(dict_freeradius), tmpl_attr_tail_unresolved(tmpl))) { - if (fr_dict_attr_add(fr_dict_unconst(dict_freeradius), fr_dict_root(dict_freeradius), - tmpl_attr_tail_unresolved(tmpl), 0, FR_TYPE_UINT64, flags) < 0) { + if (fr_dict_attr_add_name_only(fr_dict_unconst(dict_freeradius), fr_dict_root(dict_freeradius), + tmpl_attr_tail_unresolved(tmpl), FR_TYPE_UINT64, flags) < 0) { cf_log_perr(conf, "Failed defining %s attribute", name); return -1; }