Re: [PATCH 1/10] Proper file name escaping in Autoconf programs and Perl modules.

[Ralf Wildenhues]

Hello,

* Russ Allbery wrote on Tue, Dec 04, 2007 at 10:48:13PM CET:
> Ralf Wildenhues <address@hidden> writes:
> 
> > This includes escaping of characters special to the shell as well as
> > special to Perl, e.g., leading `<' or `>'.  For example, when $file
> > starts with `>', `open ">$file"' wrongly tries to append to a different
> > file.
> 
> The best fix here from a Perl perspective would be to use the
> three-argument form of open everywhere rather than just appending
> characters to the file name argument of the two-argument form.  You then
> require Perl 5.006, but that's not a bad requirement these days.

Probably a good idea, but I'm currently in the mood to fix things in a
conservative way.  I suppose if somebody did this work it would be
appreciated.

> When using the two-argument form, files with leading or trailing
> whitespace will still cause you problems even with an explicit mode
> character always prepended.

AFAICS trailing white space is not a problem.  Leading white space can
be fixed with Paul's suggestion:

* Paul Eggert wrote on Tue, Dec 04, 2007 at 11:43:15PM CET:
> Ralf Wildenhues <address@hidden> writes:
> 
> > but I wouldn't know how to make it foolproof.  First, if the space is
> > missing after `>' and $file begins with `>', then append mode is used.
> > But also, if $file begins with whitespace, that is just ignored anyway.
> 
> I'm a bit out of context here, but can't you prepend "./" to the file
> name, if the file begins with " " or ">" or any other troublesome
> character?

* Paul Eggert wrote on Wed, Dec 05, 2007 at 12:12:02AM CET:
> Ralf Wildenhues <address@hidden> writes:
> 
> > +sub shell_quote($)
> > +{
> > +  my ($s) = @_;
> > +  if ($s =~ m![^\w+/.,-]!)
> > +    {
> > +      # Convert each single quote to '\''
> > +      $s =~ s/\'/\'\\\'\'/g;
> > +      # Then single quote the string.
> > +      $s = "'$s'";
[...]
> A nit: if the goal is to quote only those file names requiring
> quoting, then this quotes too often; e.g., it quotes @ in file names.
> I think the only characters you should need to worry about in practice
> are:
> 
> \t \n \r space ! " # $ % & ' ( ) * ; < > ? [ \ ^ ` | ~

I don't mind if it quotes too often.  Of course a patch to refine that
would be welcome, but the general idea is merely to not quote in the
common case, so that we don't gratuitously run into avoidable line
length limitations upon exec.

* Eric Blake wrote on Wed, Dec 05, 2007 at 02:23:03PM CET:
> According to Ralf Wildenhues on 12/4/2007 2:45 PM:
> > +AT_SETUP([autom4te and whitespace in file names])
> > +
> > +file='>file with  funny \ '\'' \'\'' $ & #!*? name'
> > +dir='>dir with  funny \ '\'' \'\'' $ & #!*? name'
> 
> I'm wondering if it is worth splitting this one test into two, in order to
> get more coverage on Windows ports - one that tests file names that can be
> created by default on any windows platform, and one that covers the
> remaining characters that can only be created in a cygwin managed mount.

Thanks, done in the updated patch coming up in another mail.  FWIW, I
did not know which characters were w32-safe.

> In other words, ', $, &, #, !, and whitespace are safe, but \, " (did you
> miss that on purpose?), <, >, *, and ? are not.

I missed " by accident, added now.  Thanks.

> > +# skip if we cannot create such a file or directory
> > +AT_CHECK([mkdir "$dir" "$cachedir" "$TMPDIR" && test -f "$file.m4" || exit 
> > 77])
> 
> Unrelated to your patch series, but I'm wondering if we should provide
> AT_SKIP or AT_SKIP_IF that uses the file system to record skips, so that
> you can expand such a macro regardless of how many subshells you are
> nested in, and not have to worry about exit status 77 propagating all the
> way out to the test engine.

Sure.  I'm not quite sure how this would look like, though; write to a
special at-skip file?  Which instance would test that, then, if the
AT_SKIP_IF appeared outside of any AT_CHECK?

> On the other hand, I know the tar testsuite already defines its own
> AT_SKIP.

AFAICS current CVS sources define AT_SKIP_TEST, not AT_SKIP.

Cheers,
Ralf