[Rpm-maint] [PATCH] Check for undefined macros

[Panu Matilainen]

On 01/27/2013 07:27 AM, Alexey Tourbin wrote:
> This change introduces a generalized routine rpmExpandMacros() to expand
> macros on behalf of user input, with improved diagnostic facilities.
> In particular, one of its arguments is a callback function which is
> called whenever an undefined macro is encountered in user input.
>
> When specfile parser calls this routine, it supplies a custom function
> to handle undefined macros.  This function is keenly aware of specfile
> syntax: valid specfile tokens, such as %prep and %setup, when found in
> the right context, are not treated as undefined macros.  Otherwise,
> there are 3 possibilities.
> - Undefined macros in preamble and %pre/%post/... scriptlets raise
>    an error, unless a non-build parse is performed; this is to prevent
>    malformed packages from being built.
> - In other cases, a warning is issued.
> - Some tokens are permitted beyond their context (e.g. %ghost in
>    %changelog), to comply with traditional/sloppy usage.  Also, undefined
>    macros found in comments normally do not produce a warning.

Trying to finally say something meaningful about this...

First of all, I certainly do like the direction this is going to. I've 
been thinking about adding some way to register the spec keywords and 
pseudo-macros into the macro parser but never got around to do anything 
beyond thinking level.

You can probably guess by now what I'm going to say first: this needs 
splitting up :) See below...

> -int expandMacros(void * spec, rpmMacroContext mc, char * sbuf, size_t slen)
> -{
> -    char *target = NULL;
> -    int rc = doExpandMacros(mc, sbuf, &target);
> -    rstrlcpy(sbuf, target, slen);
> -    free(target);
> -    return rc;
> +    free(mb.buf);
> +    return NULL;
>   }

Please leave expandMacros() around for the time being, as a similar 
dummy wrapper as it is now. It needs to go eventually, but removing it 
would force a soname bump and we really dont want to do that just for 
this silly little thing. Keeping it also allows trivially splitting this 
huge commit up a bit: one patch to change the macro side, another to 
change the build code to take advantage of the new interface instead of 
expandMacros(). After that, expandMacros() can be marked for axing on 
the next soname bump, whenever that happens.

> +char * rpmExpandMacros	(rpmMacroContext mc, const char *src,
> +				const char *fileName, int lineNum,
> +				void undefined(const char *fileName, int lineNum,
> +					const char *s, const char *f, const char *fe,
> +					const char *exp, int level, void *arg),
> +				void *arg);

Eek :) This is quite an argument monster for a public API function. Most 
of it is of course actually for the undefined() callback, and could be 
hidden behind a typedef to make it less scary-looking and to require 
less arguments for common uses - I'm basically shopping for something 
that could nicely replace many uses of rpmExpand(), see below. Haven't 
looked in detail how feasible (or not) these would be in practise but 
some ideas / thoughts:

The callback could perhaps be further hidden away in the API with 
setCallback() style function. Even if not, I think it'd make sense to 
lump the callback arguments into a "callback data" struct where 
fileName, lineNum and arg (for caller private data) are passed along so 
you dont need so many individual arguments for the function itself and 
the callback.

On a related note, since fileName and lineNum are really only used for 
error reporting, perhaps it should instead have a retval argument for 
returning the error message string, and leave reporting of file names & 
line numbers to the caller.

Then there's the idea of actually being able to "register" new built-in 
macros into the macro engine. Looking at undefined() in parseSpec.c, 
that's likely not possible to handle by registering spec keywords as 
macros with a custom callback for each case, but OTOH the %files perhaps 
could. But perhaps the bigger advantage would be allowing eg %setup to 
be a real built-in macro instead of the pseudo-macro-foobar it is now.

Finally (although I've a feeling I'm forgetting something here but this 
getting a bit long as it is...), while we're adding a new enhanced macro 
expansion function, I'd like to see a "flags" argument included to allow 
controlling some behaviors, even if unused right now (I'm certainly not 
expecting you to implement any of this, just a brain-dump of what kinds 
of things I have in mind :)

Some possible (future) flags could be:
- a "strict" flag where warnings become errors (vague I know, but 
remember these are just ideas :)
- "readonly" flag where attempt to define or undefine new macros is an 
error / warning and wont be honored
- "noexec" flag where attempt to run shell/lua code is an error

The latter two are kinda security oriented things: rpmExpand() is used 
for grabbing all sorts of configuration settings deep in transactions 
while running with "god powers" (root, unrestricted in selinux etc). 
Those configuration macros are expected to be "simple" things like 
constants or just %{_bindir} -> /usr/bin style expansions, but nothing 
restricts them from executing shell/lua and doing nasty things (whether 
accidentally or on evil purpose).

	- Panu -