From: Alan T. DeKok Date: Mon, 20 Sep 2021 12:59:58 +0000 (-0400) Subject: add code and test cases for copying enums X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8bdf27e992278e0737b98cd20268743df642a9ac;p=thirdparty%2Ffreeradius-server.git add code and test cases for copying enums even between attributes of different data types --- diff --git a/src/lib/util/dict_fixup.c b/src/lib/util/dict_fixup.c index ac4865c432..b69fe896d3 100644 --- a/src/lib/util/dict_fixup.c +++ b/src/lib/util/dict_fixup.c @@ -383,21 +383,13 @@ int dict_fixup_clone(dict_fixup_ctx_t *fctx, char const *filename, int line, char const *ref, size_t ref_len) { dict_fixup_clone_t *fixup; - fr_dict_attr_t const *target; /* - * As a quick check, see if the types are compatible. + * Delay type checks until we've loaded all of the + * dictionaries. This means that errors are produced + * later, but that shouldn't matter for the default + * dictionaries. They're supposed to work. */ - target= fr_dict_attr_by_oid(NULL, fr_dict_root(da->dict), ref); - if (target) { - if (target->type != da->type) { - fr_strerror_printf("Clone reference MUST be to an attribute of type '%s' at [%s:%d]", - fr_table_str_by_value(fr_value_box_type_table, target->type, ""), - filename, line); - return -1; - } - } - fixup = talloc(fctx->pool, dict_fixup_clone_t); if (!fixup) { fr_strerror_const("Out of memory"); @@ -440,7 +432,7 @@ static inline CC_HINT(always_inline) int dict_fixup_clone_apply(UNUSED dict_fixu for (parent = fixup->da->parent; !parent->flags.is_root; parent = parent->parent) { if (parent == da) { - fr_strerror_printf("Clone references MUST NOT refer to a parent attribute %s at %s[%d]", + fr_strerror_printf("References MUST NOT refer to a parent attribute %s at %s[%d]", parent->name, fr_cwd_strip(fixup->common.filename), fixup->common.line); return -1; } @@ -451,20 +443,63 @@ static inline CC_HINT(always_inline) int dict_fixup_clone_apply(UNUSED dict_fixu if (!da) return -1; } + if (fr_dict_attr_ref(da)) { + fr_strerror_printf("References MUST NOT refer to an ATTRIBUTE which itself has a 'ref=...' at %s[%d]", + fr_cwd_strip(fixup->common.filename), fixup->common.line); + return -1; + } + /* * We can only clone attributes of the same data type. + * + * @todo - for ENUMs, we have to _manually_ clone the + * values if the types are different. This means looping + * over the ref da, and _casting_ the values to the new + * data type. If the cast succeeds, we add the value. + * Otherwise we don't. */ if (da->type != fixup->da->type) { - fr_strerror_printf("Clone reference MUST be to an attribute of type '%s' at %s[%d]", - fr_table_str_by_value(fr_value_box_type_table, fixup->da->type, ""), - fr_cwd_strip(fixup->common.filename), fixup->common.line); - return -1; - } + int copied; - if (fr_dict_attr_ref(da)) { - fr_strerror_printf("Clone references MUST NOT refer to an ATTRIBUTE which has 'ref=...' at %s[%d]", - fr_cwd_strip(fixup->common.filename), fixup->common.line); - return -1; + /* + * Structural types cannot be the source or destination of clones. + * + * Leaf types can be cloned, even if they're + * different types. But only if they don't have + * children (i.e. key fields). + */ + if (fr_type_is_non_leaf(da->type) || fr_type_is_non_leaf(fixup->da->type) || + dict_attr_children(da) || dict_attr_children(fixup->da)) { + fr_strerror_printf("Reference MUST be to a simple data type of type '%s' at %s[%d]", + fr_table_str_by_value(fr_value_box_type_table, fixup->da->type, ""), + fr_cwd_strip(fixup->common.filename), fixup->common.line); + return -1; + } + + /* + * We copy all of the VALUEs over from the source + * da by hand, by casting them. + * + * We have to do this work manually because we + * can't call dict_attr_acopy(), as that function + * copies the VALUE with the *source* data type, + * where we need the *destination* data type. + */ + copied = dict_attr_acopy_enumv(fixup->da, da); + if (copied < 0) return -1; + + if (!copied) { + fr_strerror_printf("Reference copied no VALUEs from type type '%s' at %s[%d]", + fr_table_str_by_value(fr_value_box_type_table, fixup->da->type, ""), + fr_cwd_strip(fixup->common.filename), fixup->common.line); + return -1; + } + + /* + * We don't need to copy any children, so leave + * fixup->da in the dictionary. + */ + return 0; } /* @@ -473,19 +508,20 @@ static inline CC_HINT(always_inline) int dict_fixup_clone_apply(UNUSED dict_fixu */ cloned = dict_attr_acopy(dict->pool, da, fixup->da->name); if (!cloned) { - fr_strerror_printf("Failed cloning attribute '%s' to %s", da->name, fixup->ref); + fr_strerror_printf("Failed copying attribute '%s' to %s", da->name, fixup->ref); return -1; } cloned->attr = fixup->da->attr; cloned->parent = fixup->parent; /* we need to re-parent this attribute */ + cloned->depth = cloned->parent->depth + 1; /* * Copy any pre-existing children over. */ if (dict_attr_children(fixup->da)) { if (dict_attr_acopy_children(dict, cloned, fixup->da) < 0) { - fr_strerror_printf("Failed cloning attribute '%s' from children of %s", da->name, fixup->ref); + fr_strerror_printf("Failed copying attribute '%s' from children of %s", da->name, fixup->ref); return -1; } } @@ -495,12 +531,12 @@ static inline CC_HINT(always_inline) int dict_fixup_clone_apply(UNUSED dict_fixu */ if (dict_attr_children(da)) { if (dict_attr_acopy_children(dict, cloned, da) < 0) { - fr_strerror_printf("Failed cloning attribute '%s' from children of %s", da->name, fixup->ref); + fr_strerror_printf("Failed copying attribute '%s' from children of %s", da->name, fixup->ref); return -1; } if (dict_attr_child_add(fr_dict_attr_unconst(fixup->parent), cloned) < 0) { - fr_strerror_printf("Failed adding cloned attribute %s", da->name); + fr_strerror_printf("Failed adding attribute %s", da->name); talloc_free(cloned); return -1; } diff --git a/src/lib/util/dict_priv.h b/src/lib/util/dict_priv.h index 88ffe60cd9..5345ef694a 100644 --- a/src/lib/util/dict_priv.h +++ b/src/lib/util/dict_priv.h @@ -173,6 +173,8 @@ fr_dict_attr_t *dict_attr_acopy(TALLOC_CTX *ctx, fr_dict_attr_t const *in, char int dict_attr_acopy_children(fr_dict_t *dict, fr_dict_attr_t *dst, fr_dict_attr_t const *src); +int dict_attr_acopy_enumv(fr_dict_attr_t *dst, fr_dict_attr_t const *src); + int dict_attr_child_add(fr_dict_attr_t *parent, fr_dict_attr_t *child); int dict_protocol_add(fr_dict_t *dict); diff --git a/src/lib/util/dict_util.c b/src/lib/util/dict_util.c index b05f397bcc..96e8228a13 100644 --- a/src/lib/util/dict_util.c +++ b/src/lib/util/dict_util.c @@ -746,6 +746,62 @@ int dict_attr_acopy_children(fr_dict_t *dict, fr_dict_attr_t *dst, fr_dict_attr_ return 0; } +/** Copy the VALUEs of an existing attribute, by casting them + * + * @param[in] dst where to cast the VALUEs to + * @param[in] src where to cast the VALUEs from + * @return + * - 0 on success (but copied no values) + * - 1 on success (but copied at least one value) + * - <0 on error + */ +int dict_attr_acopy_enumv(fr_dict_attr_t *dst, fr_dict_attr_t const *src) +{ + fr_dict_enum_value_t const *enumv; + fr_dict_attr_ext_enumv_t *ext; + fr_hash_iter_t iter; + int copied = 0; + + fr_assert(!fr_type_is_non_leaf(dst->type)); + fr_assert(!fr_type_is_non_leaf(src->type)); + + fr_assert(!fr_dict_attr_is_key_field(dst)); + fr_assert(!fr_dict_attr_is_key_field(src)); + + fr_assert(fr_dict_attr_has_ext(dst, FR_DICT_ATTR_EXT_ENUMV)); + fr_assert(fr_dict_attr_has_ext(src, FR_DICT_ATTR_EXT_ENUMV)); + + ext = fr_dict_attr_ext(src, FR_DICT_ATTR_EXT_ENUMV); + if (!ext) { + fr_assert(0); + return -1; + } + + if (!ext->name_by_value) { + fr_strerror_printf("Reference enum %s does not have any VALUEs to copy", src->name); + return -1; + } + + /* + * Loop over the VALUEs, adding names from the old + * attribute to the new one. + * + * If a value can't be cast, then just ignore it. + */ + for (enumv = fr_hash_table_iter_init(ext->name_by_value, &iter); + enumv; + enumv = fr_hash_table_iter_next(ext->name_by_value, &iter)) { + if (dict_attr_enum_add_name(dst, enumv->name, enumv->value, true, + false, NULL) < 0) { + continue; + } + + copied++; + } + + return copied; +} + /** Add a protocol to the global protocol table * * Inserts a protocol into the global protocol table. Uses the root attributes diff --git a/src/tests/dict/base.dict b/src/tests/dict/base.dict index 7b4e4fcd7c..76fb9b460a 100644 --- a/src/tests/dict/base.dict +++ b/src/tests/dict/base.dict @@ -37,6 +37,9 @@ VALUE base-enum-uint64 three 3 ATTRIBUTE Base-Integer64 19 uint64 enum=base-enum-uint64 ATTRIBUTE Base-IPv4-Prefix 20 ipv4prefix +# and this casting should work +ATTRIBUTE Base-Integer32 21 uint32 enum=base-enum-uint64 + # Ignore VSA, VENDOR, timeval, boolean, combo-ip-prefix, decimal... for now diff --git a/src/tests/unit/dictionary b/src/tests/unit/dictionary index 2a50bd2065..f835647113 100644 --- a/src/tests/unit/dictionary +++ b/src/tests/unit/dictionary @@ -13,6 +13,15 @@ BEGIN-PROTOCOL RADIUS # of the standard RADIUS dictionary. # +# +# Define an ENUM +# +ENUM base-enum-uint64 uint64 +VALUE base-enum-uint64 one 1 +VALUE base-enum-uint64 two 2 +VALUE base-enum-uint64 three 3 + + ATTRIBUTE Unit-String 248 string ATTRIBUTE Unit-Enumv 249 integer @@ -39,6 +48,11 @@ ATTRIBUTE Delta-Sec-uint32 254.9 time_delta seconds,uint32 ATTRIBUTE Delta-MSec-int32 254.10 time_delta milliseconds,int32 ATTRIBUTE Delta-Sec-int32 254.11 time_delta seconds,int32 +ATTRIBUTE Test-Enum-Integer64 254.12 uint64 enum=base-enum-uint64 + +# and this casting should work +ATTRIBUTE Test-Enum-Integer32 254.13 uint32 enum=base-enum-uint64 + # # Copied here for simplicity # diff --git a/src/tests/unit/protocols/radius/enum.txt b/src/tests/unit/protocols/radius/enum.txt new file mode 100644 index 0000000000..96073164d0 --- /dev/null +++ b/src/tests/unit/protocols/radius/enum.txt @@ -0,0 +1,30 @@ +# +# ENUM tests +# +# src/tests/dict checks if the dictionaries can be parsed. +# This file tests if the contents are OK. +# +proto radius +proto-dictionary radius + +encode-pair Unit-TLV = { Test-Enum-Integer64 = one } +match fe 0c 0c 0a 00 00 00 00 00 00 00 01 + +# +# @todo - find out why this isn't an enum lookup :( +# +decode-pair - +match Unit-TLV = { Test-Enum-Integer64 = 1 } + +# +# Same enum names, different attribute. +# +encode-pair Unit-TLV = { Test-Enum-Integer32 = one } +match fe 08 0d 06 00 00 00 01 + +# This value is looked up as an an enum! +decode-pair - +match Unit-TLV = { Test-Enum-Integer32 = one } + +count +match 10