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

[Panu Matilainen]

On 02/10/2013 01:16 PM, 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}
>
> 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.
>
> In the meanwhile somebody has spotted the ownership issue and
> brute-forced it:
> http://pkgs.fedoraproject.org/cgit/amanda.git/commit/?h=f18
>
> A better fix would've been just to change the nested %defines into
> %globals, eg:
>
> %{!?amanda_user:%global amanda_user amandabackup}
>
> ...which places the %amanda_user macro in global scope as the original
> intention was, ensuring it wont get mopped out by a parametrized macro.
>
>> 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...
>
> Thinking about it again, we could at least issue warnings about it.

Another possiblity might be a more subtle change towards the behavior 
that people generally expect: %define could be made to use global scope 
when used outside parametrized macro expansion.

That would continue to allow user defined "local variable" macros inside 
parameterized macros as they are now, but make the common
"%{!?foo:%define foo bar}" idiom in specs work reliably.

That is of course a fundamental "language" change too and could break 
somebody's legal macro constructs. It does seem unlikely, considering 
the current wacky behavior wrt user-defined "local" macros, but then you 
never know... It would also introduce another compatibility issue where 
specs relying on the new behavior would silently break on older rpm 
versions, and silent breakage is never nice.

Dunno, thoughts / opinions / better ideas welcome.

	- Panu -