Hi!
On Fri, Nov 07, 2025 at 04:19:37PM +0900, Hyeonggeun Oh wrote:
> Currently, variable names are only used during parsing and are not
> stored at runtime. This makes it impossible to iterate through
> variables and retrieve their names.
>
> This patch adds infrastructure to store variable names:
> - Add 'name' and 'name_len' fields to var_desc structure
> - Add 'name' field to var structure
Oh, I had totally forgotten that the names were not stored! Indeed,
we only keep a hash. Originally we didn't want to reserve more
memory for names because we had no idea how this would evolved and
preferred to stay on the conservative side. But after 10 years of
variables, we can confidently say that the vast majority of the
configs are way below 1000 variables and that this is just nothing
in terms of memory usage. We've seen a few bad cases where they
were massively created from Lua (and not even purged), using random
names, but there's nothing that can be done to help these ones anyway
and it's not just an extra name that will make a difference.
> - Add VDF_NAME_ALLOCATED flag to track memory ownership
> - Store names in vars_fill_desc(), var_set(), vars_check_arg(),
> and parse_store()
> - Free names in var_clear() and release_store_rule()
Thanks for explaining the principles.
> /* This struct describes a variable as found in an arg_data */
> struct var_desc {
> uint64_t name_hash;
> enum vars_scope scope;
> uint flags; /*VDF_* */
> + const char *name; /* variable name (not owned) */
> + size_t name_len; /* variable name length */
Usually we rely on "struct ist" for (name,len) pairs, which permits to
manipulate them, but here I'm seeing that it wouldn't bring anything
at all since it's only about storing a copy of a name for later use,
so that's fine.
> diff --git a/src/vars.c b/src/vars.c
> index 878ad1a5e..a11101946 100644
> --- a/src/vars.c
> +++ b/src/vars.c
> @@ -178,6 +178,11 @@ unsigned int var_clear(struct vars *vars, struct var
> *var, int force)
> {
> unsigned int size = 0;
>
> + if (var->name) {
> + free(var->name);
> + var->name = NULL;
> + }
> +
Here's no need for the "if" here. You should even just use ha_free()
which sets the null for you (we encourage this to limit the proliferation
of isolated free() which can easily result in use-after-free situations):
ha_free(&var->name);
> @@ -431,6 +438,12 @@ int var_set(const struct var_desc *desc, struct sample
> *smp, uint flags)
> goto unlock;
> var->name_hash = desc->name_hash;
> var->flags = flags & VF_PERMANENT;
> +
> + /* Save variable name */
> + var->name = NULL;
> + if (desc->name && desc->name_len > 0) {
> + var->name = strndup(desc->name, desc->name_len);
> + }
strndup() isn't always present on the systems that we support, and instead
we have our own local implementation: my_strndup(), that is preferred for
this reason.
> @@ -634,15 +647,28 @@ int vars_check_arg(struct arg *arg, char **err)
> if (!vars_fill_desc(arg->data.str.area, arg->data.str.data, &desc, err))
> return 0;
>
> - if (desc.scope == SCOPE_PROC && !var_set(&desc, &empty_smp,
> VF_CREATEONLY|VF_PERMANENT))
> + /* Save variable name before destroying the chunk */
> + char *saved_name = NULL;
Please do not mix variables declaration with code, they significantly
complicate code reading later. Just declare saved_name at the beginning
of the code block or the function.
> + if (desc.name && desc.name_len > 0) {
> + saved_name = malloc(desc.name_len + 1);
> + if (saved_name) {
> + memcpy(saved_name, desc.name, desc.name_len);
> + saved_name[desc.name_len] = '\0';
> + desc.name = saved_name; /* Update desc.name to point
> to saved copy */
> + }
> + }
> +
> + if (desc.scope == SCOPE_PROC && !var_set(&desc, &empty_smp,
> VF_CREATEONLY|VF_PERMANENT)) {
Hmmm if the malloc() above failed, you're passing the pointer returned by
vars_fill_desc() here in desc.name, right ? And my understanding is that
it's not what we want, as this means two pointers to the same string, which
will be a pain when freeing. Or maybe you're counting on the cases where
we're sharing a same name that came from a const somewhere, in which case
I think you could to the malloc() only for the cases where the previous
one was already allocated. Also, didn't you create VF_NAME_ALLOCATED
precisely for the case where you need to malloc() like above, which could
be missing here ?
> @@ -973,6 +1005,17 @@ static enum act_parse_ret parse_store(const char
> **args, int *arg, struct proxy
> if (!vars_fill_desc(var_name, var_len, &rule->arg.vars.desc, err))
> return ACT_RET_PRS_ERR;
>
> + /* Save variable name for runtime use */
> + if (rule->arg.vars.desc.name && rule->arg.vars.desc.name_len > 0) {
> + char *saved_name = malloc(rule->arg.vars.desc.name_len + 1);
> + if (saved_name) {
> + memcpy(saved_name, rule->arg.vars.desc.name,
> rule->arg.vars.desc.name_len);
> + saved_name[rule->arg.vars.desc.name_len] = '\0';
> + rule->arg.vars.desc.name = saved_name;
> + rule->arg.vars.desc.flags |= VDF_NAME_ALLOCATED;
> + }
> + }
> +
So same comments/questions as above regarding reuse of a non-allocated
pointer and the flag which here is present but not in the previous chunk.
Willy
