Re: [PATCH] Stricter macro substitution syntax

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 01/21/2013 10:14 PM, Alexey Tourbin wrote:
This change introduces a separate routine to parse for valid macro
names.  Valid macro names are either regular 3+ character identifiers,
or special names: "S", "P", "0", "#", "*", "**", macro options such as
"-o" and "-o*", and macro arguments such as "1".  Other names are not
valid.  This fixes a number of bugs seen earlier due to sloppy name
parsing: "%_libdir*" and "%01" were not expanded (these are now expanded
to e.g. "/usr/lib64*" and "<name>1", as expected).  This also fixes
bugs in as-is substitution: "%!foo" was expanded to "%foo", and likewise
"%!!!" was expanded to "%" (and to "%<garbage>" at EOL).

Also, bad names in %name and %{name...} substitutions are now handled
differently.  In %name form, the name is parsed tentatively; a silent
fall-back to as-is substitution is provisioned when no valid name can
be obtain.  In %{name...} form, a failure to obtain a valid name is now
a syntax error.  Furthermore, only 3 variants are syntactically valid:
%{name} proper, %{name:...}, and %{name ...}.  This renders invalid
ambiguous macro substitutions such as the one found in FC18 lvm2.spec:

Requires: util-linux >= %{util-linux_version}
error: Invalid macro syntax: %{util-linux_version}

Nice. The garbage spotted by this in Fedora specs is ... scary.

OTOH changes of this scale in the macro parser are a bit scary in themselves as its sooooo easy to break some obscure corner-case :) I think I'll try to give this a spin with some of the more macro-intensive packages like Fedora's kernel and font-packages first, but unless I find something unexpected breakage, consider it applied. On a related note, the test-suite could really use more thorough and more advanced test-cases to make these kind of changes less scary.

BTW, while the rules are obviously different between define vs expansion due to the built-in special macros, perhaps parseMacroName() function here could be extended to cover the %define/%global/%undefine macro name sanity checks as well (see my reply wrt the missing-whitespace patch).

	- Panu -

---
  rpmio/macro.c | 164 +++++++++++++++++++++++++++++++++++++++-------------------
  1 file changed, 111 insertions(+), 53 deletions(-)

diff --git a/rpmio/macro.c b/rpmio/macro.c
index 9229580..2abdbd4 100644
--- a/rpmio/macro.c
+++ b/rpmio/macro.c
@@ -1011,6 +1011,93 @@ doFoo(MacroBuf mb, int negate, const char * f, size_t fn,
  }

  /**
+ * Parse for flags before macro name, such as in %{?!name:subst}.
+ * @param s		macro flags start
+ * @param negate	flipped by each '!' flag
+ * @param chkexist	increased by each '?' flag
+ * @return		macro flags end
+ */
+static const char *
+parseMacroFlags(const char *s, int *negate, int *chkexist)
+{
+    while (1) {
+	switch (*s) {
+	case '!':
+	    *negate = !*negate;
+	    s++;
+	    break;
+	case '?':
+	    (*chkexist)++;
+	    s++;
+	    break;
+	default:
+	    return s;
+	}
+    }
+    /* not reached */
+    return s;
+}
+
+/**
+ * Parse for valid macro name (during expansion).
+ * @param s		macro name start
+ * @return		macro name end, NULL on error
+ */
+static const char *
+parseMacroName(const char *s)
+{
+    /* alnum identifiers */
+    if (risalpha(*s) || *s == '_') {
+	const char *se = s + 1;
+	while (risalnum(*se) || *se == '_')
+	    se++;
+	switch (se - s) {
+	case 1:
+	    /* recheck for [SPF] */
+	    break;
+	case 2:
+	    return NULL;
+	default:
+	    return se;
+	}
+    }
+    /* simple special names */
+    switch (*s) {
+    case '0':
+    case '#':
+    case 'S':
+    case 'P':
+    case 'F':
+	s++;
+	return s;
+    case '*':
+	s++;
+	if (*s == '*')
+	    s++;
+	return s;
+    /* option names */
+    case '-':
+	s++;
+	if (risalnum(*s))
+	    s++;
+	else
+	    return NULL;
+	if (*s == '*')
+	    s++;
+	return s;
+    }
+    /* argument names */
+    if (risdigit(*s)) {
+	s++;
+	while (risdigit(*s))
+	    s++;
+	return s;
+    }
+    /* invalid macro name */
+    return NULL;
+}
+
+/**
   * The main macro recursion loop.
   * @param mb		macro expansion state
   * @param src		string to expand
@@ -1083,34 +1170,14 @@ expandMacro(MacroBuf mb, const char *src, size_t slen)
  	chkexist = 0;
  	switch ((c = *s)) {
  	default:		/* %name substitution */
-		while (strchr("!?", *s) != NULL) {
-			switch(*s++) {
-			case '!':
-				negate = ((negate + 1) % 2);
-				break;
-			case '?':
-				chkexist++;
-				break;
-			}
-		}
-		f = se = s;
-		if (*se == '-')
-			se++;
-		while((c = *se) && (risalnum(c) || c == '_'))
-			se++;
-		/* Recognize non-alnum macros too */
-		switch (*se) {
-		case '*':
-			se++;
-			if (*se == '*') se++;
-			break;
-		case '#':
-			se++;
-			break;
-		default:
-			break;
+		f = parseMacroFlags(s, &negate, &chkexist);
+		fe = parseMacroName(f);
+		/* no valid name? assume as-is substitution */
+		if (fe == NULL) {
+			mbAppend(mb, '%');
+			continue;
  		}
-		fe = se;
+		se = fe;
  		/* For "%name " macros ... */
  		if ((c = *fe) && isblank(c))
  			if ((lastc = strchr(fe,'\n')) == NULL)
@@ -1140,48 +1207,39 @@ expandMacro(MacroBuf mb, const char *src, size_t slen)
  			rc = 1;
  			continue;
  		}
-		f = s+1;/* skip { */
+		f = parseMacroFlags(s + 1 /* skip { */, &negate, &chkexist);
+		fe = parseMacroName(f);
  		se++;	/* skip } */
-		while (strchr("!?", *f) != NULL) {
-			switch(*f++) {
-			case '!':
-				negate = ((negate + 1) % 2);
-				break;
-			case '?':
-				chkexist++;
-				break;
-			}
+		/* no valid name? syntax error */
+		if (fe == NULL) {
+			rpmlog(RPMLOG_ERR,
+				_("Invalid macro name: %%%.*s\n"), (int)(se - s), s);
+			rc = 1;
+			continue;
  		}
-		for (fe = f; (c = *fe) && !strchr(" :}", c);)
-			fe++;
-		switch (c) {
+		switch (*fe) {
  		case ':':
  			g = fe + 1;
  			ge = se - 1;
  			break;
  		case ' ':
+		case '\t':
  			lastc = se-1;
  			break;
-		default:
+		case '}':
  			break;
+		default:
+			rpmlog(RPMLOG_ERR,
+				_("Invalid macro syntax: %%%.*s\n"), (int)(se - s), s);
+			rc = 1;
+			continue;
  		}
  		break;
  	}

-	/* XXX Everything below expects fe > f */
+	assert(fe > f);
  	fn = (fe - f);
  	gn = (ge - g);
-	if ((fe - f) <= 0) {
-/* XXX Process % in unknown context */
-		c = '%';	/* XXX only need to save % */
-		mbAppend(mb, c);
-#if 0
-		rpmlog(RPMLOG_ERR,
-			_("A %% is followed by an unparseable macro\n"));
-#endif
-		s = se;
-		continue;
-	}

  	if (mb->macro_trace)
  		printMacro(mb, s, se);


_______________________________________________
Rpm-list mailing list
Rpm-list@xxxxxxxxxxxxx
http://lists.rpm.org/mailman/listinfo/rpm-list


[Index of Archives]     [RPM Ecosystem]     [Linux Kernel]     [Red Hat Install]     [PAM]     [Red Hat Watch]     [Red Hat Development]     [Red Hat]     [Gimp]     [Yosemite News]     [IETF Discussion]

  Powered by Linux