[Rpm-maint] [PATCH 5/5] Fix horrible bug in freeing macro arguments

[Alexey Tourbin]

On Sun, Feb 10, 2013 at 01:16:47PM +0200, Panu Matilainen wrote:
> On 02/05/2013 06:31 AM, Alexey Tourbin wrote:
> >After a macro which takes options/arguments is called, such as %systemd_post,
> >freeArgs() frees ALL macros *previously defined in specfile*.  For example,
> >in amanda.spec, after scriptlet sections which invoke %systemd_* macros are
> >processed, %amanda_user and %amanda_group in %files sections become undefined,
> >resulting in malformed owner/group file metadata.
> >
> >$ rpm -qp --qf '[%{FILEUSERNAME}:%{FILEGROUPNAME}\n]' amanda-3.3.2-2.fc18.x86_64.rpm |
> >>sort | uniq -c | sort -n | tail
> >       3 %amanda_user:%amanda_group
> >     104 root:root
> 
> Nope. If you think about it, next to nothing would work if there was
> such a bug, really.
> 
> This is the classic rpm "local macro scope" gotcha:
> 
> %{!?amanda_user:%define amanda_user amandabackup}

Ah! I see now.

> Since the define occurs inside a %{} "block", %amanda_user doesn't
> get defined in the global scope (or level 0 in rpm macro-speak)
> which you'd kinda expect but "local" level 1. Technically the
> %amanda_user macro should fall out of scope at the closing }, but
> these are only "garbage collected" when a parametrized macro is
> executed. Which leads to wonderfully obscure behavior and packaging
> bugs repeated again and again... It's also the reason why Fedora's
> packaging guidelines recommend using %global "everywhere", which in
> turn leads to other issues as %global differs from %define in other
> ways.

Is it documented that defines in a "block" are local to the block?
I think I had some thought about that, because there's a comment in
freeArgs() which says "Delete dynamic macro definitions".  On the other
hand, freeArgs() is complementary to grabArgs(), and their sole purpose,
as annotated by javadoc comments, is to set up and later tear down macro
options and arguments.  Nothing else in the code, it seems, betrays the
idea that defines must be somehow local to a block.

Using %global everywhere is bad idea, because global evaluates the body,
which is usually not intended.  Typically global needs to be used only
when a previous value must be amended:

	%global PATH %{PATH}:/usr/local/bin

This will not work with %define, due to recursion.

> >This change installs additional name check in freeArgs(), so that, upon
> >finishing macro call, only macro arguments are freed.
> 
> NAK. This fundamentally changes the macro "language" behavior. Sort
> of like changing scope of all variables regardless of the place of
> declaration to global in C, but since this is rpm, with extra
> twists...
> 
> I do agree the behavior is horrible, and it would be good to do
> something about it. I once tried to fix it by enforcing the "local
> scoping" for non-parametrized macros but it broke some complex but
> legitimate-looking macros in turn and didn't have the time and/or
> energy to chase it down. And since then largely forgotten about
> it...

Yes, from the "language" point of view, the right thing to do then is to
remove local definitions as soon as they go out of scope.  But since the
language is loosely defined...

> Thinking about it again, we could at least issue warnings about it.
> Not sure this is entirely correct, but for the handful of test-cases
> I threw at it I didn't spot any false positives:
> 
> --- a/rpmio/macro.c
> +++ b/rpmio/macro.c
> @@ -1324,6 +1324,11 @@ expandMacro(MacroBuf mb, const char *src,
> size_t slen)
>                 continue;
>         }
> 
> +       if (me->level >= mb->depth) {
> +               rpmlog(RPMLOG_WARNING,
> +                       _("macro %s used out of scope\n"), me->name);
> +       }
> +
>         /* Setup args for "%name " macros with opts */
>         if (me && me->opts != NULL) {
>                 if (lastc != NULL) {
> 
> With that, the buggy version of amanda.spec gives:
> [pmatilai at turre rpm]$ ./rpmspec --parse ~/dist/amanda/*.spec > /dev/null
> warning: macro indexserver used out of scope
> warning: macro tapeserver used out of scope
> warning: macro amanda_user used out of scope
> warning: macro amanda_group used out of scope
> warning: macro amanda_group used out of scope
> warning: macro amanda_user used out of scope
> warning: macro amanda_user used out of scope
> [pmatilai at turre rpm]$
> 
> With the "local" %defines changed to %globals (or conditionals
> removed) the warnings are gone.