Re: Automake and whitespace in pwd

[Ralf Wildenhues]

Hi Jim,

* Jim Meyering wrote on Thu, Nov 27, 2008 at 10:55:38AM CET:
> 
> On one hand, I like the idea of accommodating the rare srcdir `pwd` that
> contains white space.

As Ralf alreay noted, it is not that rare with w32 users.

> On the other hand, since so many Makefile.am files
> are unprepared for the resulting names, I wonder if it's better to protect
> the users, and give Makefile.am writers a way to guarantee that "bad"
> `pwd` won't leak into places like $(top_srcdir).

But one point of my patch is exactly that it ensures that $(top_srcdir)
is safe to use *without* any quoting.  It has to be: it is not possible
(portably) to quote file names that can appear as part of prerequisites
or targets in makefiles.

> This started with my
> auditing some Makefile.am files, adding quotes to protect against uses
> of potentially-tainted variables like $(top_srcdir).  I found that many
> uses needed additional quoting.  In addition, while single quoting was
> usually enough to protect them, sometimes I had to use double quotes,
> or a mix, to accommodate $$() and $$i-style shell constructs.  Relying on
> that sort of correctness is fragile, in that there's nothing to actively
> prevent regressions, unless you write build tests that exercise each
> and every Makefile rule in the offending scenario.

You should not add any quoting to those variables which can reasonably
be relative.  In this category are:
  $(srcdir), $(top_srcdir), $(builddir), $(top_builddir)

My patch ensures these variables don't contain whitespace.  Of course
the *_srcdir variables can still be absolute if the user specifies an
absolute path to `configure', but the sanity check will then abort if
those absolute names contain whitespace.

> What do you think of adding an automake option that would let the package
> maintainer specify that s/he does not want to expose package users to
> the risk of malfunction due to a tainted srcdir name?

Once more, my patch will ensure that srcdir is not tainted.

These names, however, can still contain whitespace:
  $(abs_srcdir), $(abs_builddir), $(abs_top_srcdir), $(abs_top_builddir)

However, many package do not ever need to use these variables,
and Autoconf and Automake don't either (mostly), by default.
So I'm a bit reluctant to make it the default of forbidding
whitespace in them.  The most likely cause of problems will be
when packages use some variable that itself uses $MISSING, *and*
does so outside of a makefile.  Why?  Because in a shell script,
the extra 'eval' step needed for interpreting the quotation I added
is missing.  Good example is texi2dvi (which already does the right
thing by eval'ing $makeinfo).

Things are different for Libtool.  We might want to add a check
to Libtool-using packages to forbid whitespace in the absolute build
directory path.

> This feels like a package-level policy question, after all.
> Maintaining correctly-quoted Makefiles is hard enough without
> having to worry about such embedded white space.
> Sure, any package maintainer can add a few lines to configure.ac
> to make start-up code examine $srcdir and fail upon irregularity,
> but if automake were to make this easy, I'd be much more likely
> to use it than if it means copying around a custom .m4 file or
> repeating a short snippet of code in each package's configure.ac.

Again, $srcdir may not contain white space.

> That said, this patch looks fine.  "make check" passed
> and it did the right thing in a couple of manual test scenarios.

Good, thanks for testing.  I have applied it after incorporating your
NEWS suggestion, and after fixing a small portability wart, as in the
incremental patch below.  (The 'case' issue is listed in the Autoconf
manual; sigh, I think it was me who added it ...)

> P.S. Would you please re-post your latest parallelizing-tests patches?
> I missed that functionality when running automake's lengthy,
> single-threaded "make check" on a quad-core system.

Hope to get to reposting them soonish.  I've found some more issues with
them, and wanted to incorporate as many of Akim's good suggestions as
possible.

Thanks,
Ralf

diff --git a/NEWS b/NEWS
index 2721e69..8b43c69 100644
--- a/NEWS
+++ b/NEWS
@@ -138,12 +138,16 @@ New in 1.10a:
 
   - The `missing' script works better with versioned tool names.
 
+  - Automake's early configure-time sanity check now diagnoses an
+    unsafe absolute source directory name and makes configure fail.
+
   - The Automake macros and rules cope better with whitespace in the
     current directory name, as long as the relative path to `configure'
     does not contain whitespace.  To this end, the values of `$(MISSING)'
     and `$(install_sh)' may contain suitable quoting, and their expansion
     might need `eval'uation if used outside of a makefile.  These
-    undocumented variables may be used in several documented macros.
+    undocumented variables may be used in several documented macros such
+    as $(AUTOCONF) or $(MAKEINFO).
 
 Bugs fixed in 1.10a:
 
diff --git a/m4/sanity.m4 b/m4/sanity.m4
index 1704d83..3d2f304 100644
--- a/m4/sanity.m4
+++ b/m4/sanity.m4
@@ -21,11 +21,11 @@ echo timestamp > conftest.file
 am_lf='
 '
 case `pwd` in
-  *[[\"\#\$\&\'\\\`$am_lf]]*)
+  *[[\\\"\#\$\&\'\`$am_lf]]*)
     AC_MSG_ERROR([unsafe absolute working directory name]);;
 esac
 case $srcdir in
-  *[[\"\#\$\&\'\\\`$am_lf\ \   ]]*)
+  *[[\\\"\#\$\&\'\`$am_lf\ \   ]]*)
     AC_MSG_ERROR([unsafe srcdir value: `$srcdir']);;
 esac