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

[Panu Matilainen]

On 02/12/2013 09:08 AM, Alexey Tourbin wrote:
> 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.

It's ... obvious, isn't it ;)

>> 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.

The only real documentation about macros is in doc/manual/macros in the 
source tree, which does accurately document the behavior for the 
option/argument macros, but carefully neglets to mention what happens to 
user-defined macros declared inside other macros :) Except for the vague 
hint of %global defining a macro in global context, which suggests that 
other contexts exist too.

Beyond that, AFAIK the intent is only documented in various bugzilla 
comments, in particular here:
https://bugzilla.redhat.com/show_bug.cgi?id=237448#c4

...but there are numerous similar descriptions, such as in
https://bugzilla.redhat.com/show_bug.cgi?id=456389
https://bugzilla.redhat.com/show_bug.cgi?id=147238

> 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.

Unconditional "use %global everywhere" is indeed a bad recommendation 
and has led to a bunch of different kinds of bug reports, such as 
https://bugzilla.redhat.com/show_bug.cgi?id=650688

>>> 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...

Yup, like so many things in rpm, the spec and macro languages are 
largely defined by implementation instead of the other way around.
Silently removing all local definitions would make it consistent but on 
the other hand almost worse, especially as long as undefined macros are 
silently passed through (and IIRC there are some places in rpm itself 
relying on that pass-through behavior).

With the current behavior its possible to issue warnings, although if 
there's a parametrized macro expansion between define and use you dont 
get even that :-/

We could also add %local variant of define to allow explicitly stating 
the intention, but that still leaves open the question what to do about 
"local" %define :-/

	- Panu -