Hi guys,
On Wed, May 12, 2021 at 09:44:28AM +0200, Willy Tarreau wrote:
> On Wed, May 12, 2021 at 09:12:25AM +0200, Tim Düsterhus wrote:
> > Willy,
> >
> > On 5/12/21 7:47 AM, Willy Tarreau wrote:
> > > Interestingly, the code for variables was initially made for the config,
> > > so it doesn't seem to destroy variable names when they're released since
> > > that was pointless with the config. I think that code should be revisited
> > > in 2.5 to improve the situation (e.g. by marking that the variable was
> > > dynamically allocated maybe), but I don't know this part well so I'll
> > > probably stop before starting to suggest stupidities :-)
> > >
> >
> > There's also this related issue from back when I implemented this additional
> > parameter:
> >
> > https://github.com/haproxy/haproxy/issues/624
>
> Yes, and it's still unclear to me how this storage is currently arranged,
> (i.e. why only store names?) I should have a look for 2.5 probably.
OK now I got a better view of it and there is some misunderstanding in
the way the names are being used to detect if a variable exists. For
example, calling this before calling the Lua code will make it always
succeed:
http-request set-var(proc.code) int(12)
Note that the variable name here is "proc.code", not "txn.code".
What happens is that there are unified names which are independent of
the variables themselves. The principle of the names is that when
looking for a variable, we need to compare its name, and instead of
storing the name into each and every copy of a variable, there's only
a pointer to a unified location storing names that have been encountered
at least one in the process so that a single word is used in combination
with millions of variables if needed. For this, only the suffix of the
variable's name is stored, its scope is not since we already know it
when looking up a variable.
This means that the "ifexist" option shouldn't be seen as "if this
variable exists anywhere else", but as "if any variable known to the
process already caused the same suffix to be allocated".
What happens here is that while the set_var("txn.code", true) call takes
care of *not* allocating a new entry to store "code" in the names table,
get_var("txn.code") isn't as careful and will finally create it, and
notice that the variable doesn't yet exist, so it returns nil. On a
subsequent call, set_var() will find a matching suffix name and will
then store the variable, which get_var() will then find.
If we had had a get_var("sess.code"), it would also have unblocked the
situation.
In my opinion we have multiple problems here.
The first one is that if the intent of the "ifexist" was to avoid
allocating variables that are not known to the config, it doesn't work
well due to the fact that it doesn't consider the scope, so it should
be stricter and check that the variable exists with the same scope.
But how? I don't know for now.
Second, the fact that get_var() does automatically cause the creation
of that variable is by far the biggest problem, because in order to
verify if it has been filled, this will cause an allocation which will
later ensure it is always filled. So we must make it support an "ifexist"
option as well so that it is possible to perform an existence lookup
without allocating.
I suspect the set_var() modification was done for a config which uses
Lua to set a variable and where the variable was read from the config,
but that this other case where get_var() is called from Lua was
overlooked.
Last point, overall I think that the "ifexist" mechanism remains of
very limited use due to the automaticity of some of the allocations,
which were initially designed only for referencement from the config
parser. Originally, I remember that Thierry introduced a "declare var"
directive in the global section, which we found painful to use and
unnecessary due to the fact that during the parsing we already get an
exhaustive list of the variables names. But maybe for variables known
to Lua only, we should use an explicit declaration (probably from the
Lua code itself).
Thinking about it, this could correspond to just a single call to
set_var(name, "") in the init code to declare that a variable will be
used by Lua. In this case the only missing part will be taking into
consideration the scope (we could later improve that by prepending
the enum to the string in the storage for example).
So in the end I think that for 2.4 we should simply change the Lua's
get_var() so that it always uses the ifexist variant. It will at
least stop creating random names on the fly and will continue to
work with variables that have been already created by the config or
with set_var(). I don't see a single case where it makes sense to
have get_var() create a variable in your back and return NIL because
set_var() wasn't called so that next time set_var() works.
Looking more closely at vars_get_by_name(), it's only used by Lua's
various get_var() and by the CLI's "get var" that I recently added
without knowing about this unexpected behavior either.
So what I'm proposing is to simply change vars_get_by_name() to call
register_name() with alloc=0 in order to fix this mess. We can then
check during 2.5 how to refine this to also consider the scope with
the variable's name. It's just this, and fixes Joao's test case to
always return 403:
diff --git a/src/vars.c b/src/vars.c
index 996141f5d..15dcb3c3d 100644
--- a/src/vars.c
+++ b/src/vars.c
@@ -583,7 +583,7 @@ int vars_get_by_name(const char *name, size_t len, struct
sample *smp)
enum vars_scope scope;
/* Resolve name and scope. */
- name = register_name(name, len, &scope, 1, NULL);
+ name = register_name(name, len, &scope, 0, NULL);
if (!name)
return 0;
Tim, do you agree with this analysis ?
Thanks,
Willy
