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

[Panu Matilainen]

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

	- Panu -