Re: [PATCH] Again, do not change the mode of all directories below $HOME.

2008-08-02 Thread Ralf Wildenhues 
Hi Jim, and sorry for the delay,

* Jim Meyering wrote on Tue, Jul 22, 2008 at 09:42:30PM CEST:
 Ralf Wildenhues [EMAIL PROTECTED] wrote:
 
  But seriously, considering the potential for damage and mischief,
  perhaps it's time to add infrastructure that stops e.g., configure
  in its tracks whenever it detects a potentially troublesome source or
  build dir.
 
  I disagree.  You can do that and admit defeat.  I worked to mostly[1]
  fix Automake for this, because users kept complaining, and now refuse
  to admit defeat.

 I wouldn't call it admitting defeat.
 It's more like admitting that not everyone will go to
 the required lengths to make their code robust enough
 to deal with such situations.

Granted.  But please don't disallow white space in the build dir for a
normal ./configure  make all install.  That would hurt some users.

  I applied your patch, then added a test to ensure there is no further
  regression, in spite of the added cost to make distcheck.  Finally,
  I fixed a similar (and long-standing!) shell-under-quoting bug in
  test-lib.sh that was exposed by the cleanup (trap-run) code not removing
  a per-test directory for the tests that create unsearchable directories.
  With a bad build directory containing an a b component, instead of
  running e.g., chmod -R 700 /.../a b/..., the pre-patch trap/cleanup
  code would run chmod -R 700 /.../a.
 
  Would the following not have sufficed, too?
 
  -trap 'st=$?; cleanup_; d='$t_';
  -cd '$test_dir_'  chmod -R u+rwx $d  rm -rf $d  exit $st' 0
+trap 'st=$?; cleanup_; d=$t_;
+cd $test_dir_  chmod -R u+rwx $d  rm -rf $d  exit $st' 0
 
 I don't see how that would change anything.  The cd is just to
 get out of the directory we're about to remove.  It worked.
 The chmod is what failed, operating on the prefix up to,
 but not including the space.

Hmm.  I think I fail to see what exactly the shell-under-quoting was
then.  Could you be bothered to explain?  Thanks.

  BTW, I see that you use 'local' in shell functions.  This isn't required
  to be supported by POSIX sh, and AFAIK won't work with some ksh variants,
  for example the Version M 1993-12-28 r on my GNU/Linux system, but
  also your test-lib.sh doesn't check for this capability.  I suggest to
  just avoid local variables, since you have only a couple anyway.  Should
  I write a patch to this end?
 
 I'd be more amenable to a patch that makes a macro like
 posix-shell.m4 choose a shell with the features I use.
 Since no one has reported trouble yet, perhaps it's not a problem
 in practice.

Hmm.  Maybe you're right.  That would be very cool to have 'local'
available.

  FWIW, your second patch looks like it won't quite do what you intended
  if TMPDIR contains white space:
 
tp := $(shell echo $(TMPDIR)/$(PACKAGE)-)
t_prefix = $(tp)/a
... using $(t_prefix) unquoted in a number of places ...
 
 Yes.  That was deliberate.
 With those rules, I have never bothered to accommodate white space in TMPDIR.
 I expect that anyone clueful enough to run those optional rules also
 knows not to choose a TMPDIR value that is likely to cause trouble.

That is fine with me.  I just thought I'd mention it.

Cheers,
Ralf


___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils

Re: [PATCH] Again, do not change the mode of all directories below $HOME.

2008-08-02 Thread Jim Meyering 
Ralf Wildenhues [EMAIL PROTECTED] wrote:
...
  Would the following not have sufficed, too?
 
  -trap 'st=$?; cleanup_; d='$t_';
  -cd '$test_dir_'  chmod -R u+rwx $d  rm -rf $d  exit $st'   0
+trap 'st=$?; cleanup_; d=$t_;
+cd $test_dir_  chmod -R u+rwx $d  rm -rf $d  exit $st' 0

 I don't see how that would change anything.  The cd is just to
 get out of the directory we're about to remove.  It worked.
 The chmod is what failed, operating on the prefix up to,
 but not including the space.

 Hmm.  I think I fail to see what exactly the shell-under-quoting was
 then.  Could you be bothered to explain?  Thanks.

From memory, ...
when building with a space-polluted build and/or source directory name,
there was at least one failure (or mere diagnostic leading to a directory
not being removed?) that I tracked down to the failure of that chmod
command.  The chmod diagnostic mentioned the prefix before the space.

  BTW, I see that you use 'local' in shell functions.  This isn't required
  to be supported by POSIX sh, and AFAIK won't work with some ksh variants,
  for example the Version M 1993-12-28 r on my GNU/Linux system, but
  also your test-lib.sh doesn't check for this capability.  I suggest to
  just avoid local variables, since you have only a couple anyway.  Should
  I write a patch to this end?

 I'd be more amenable to a patch that makes a macro like
 posix-shell.m4 choose a shell with the features I use.
 Since no one has reported trouble yet, perhaps it's not a problem
 in practice.

 Hmm.  Maybe you're right.  That would be very cool to have 'local'
 available.

  FWIW, your second patch looks like it won't quite do what you intended
  if TMPDIR contains white space:
 
tp := $(shell echo $(TMPDIR)/$(PACKAGE)-)
t_prefix = $(tp)/a
... using $(t_prefix) unquoted in a number of places ...

 Yes.  That was deliberate.
 With those rules, I have never bothered to accommodate white space in TMPDIR.
 I expect that anyone clueful enough to run those optional rules also
 knows not to choose a TMPDIR value that is likely to cause trouble.

 That is fine with me.  I just thought I'd mention it.

Hmm... don't want to give the wrong impression.
I don't want to obfuscate the code to accommodate a pathological TMPDIR
value.  However, it'd be far better to make things fail with a sensible
diagnostic when TMPDIR has an invalid value, than to continue on
and misbehave.

too little time...


___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils

Re: [PATCH] Again, do not change the mode of all directories below $HOME.

2008-07-22 Thread Philip Rowlands 

On Tue, 22 Jul 2008, Ralf Wildenhues wrote:


* tests/CuTmpdir.pm (chmod_tree): Do not run chmod on undefined
argument, can happen when the build path contains spaces.


That sounds wrong - there's no magic to unusual characters in filenames 
other than avoiding passing them unquoted through an IFS-splitting 
shell.



sub chmod_tree
{
-  if (chdir $dir)
+  if (defined $dir  chdir $dir)
{


Surely skipping the test is going to give a misleading impression to the 
tester? Tracing back up, why is $dir not defined?



Cheers,
Phil


___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils

Re: [PATCH] Again, do not change the mode of all directories below $HOME.

2008-07-22 Thread Jim Meyering 
Philip Rowlands [EMAIL PROTECTED] wrote:
 On Tue, 22 Jul 2008, Ralf Wildenhues wrote:

 * tests/CuTmpdir.pm (chmod_tree): Do not run chmod on undefined
 argument, can happen when the build path contains spaces.

 That sounds wrong - there's no magic to unusual characters in
 filenames other than avoiding passing them unquoted through an
 IFS-splitting shell.

Hi Phil,

Ralf's change is correct.
Since the script with taint checking enabled, and mkdir requires that
its argument not be tainted, I opted to skip the test when the absolute
temporary directory name contains any unusual characters.

 sub chmod_tree
 {
 -  if (chdir $dir)
 +  if (defined $dir  chdir $dir)
 {

 Surely skipping the test is going to give a misleading impression to
 the tester? Tracing back up, why is $dir not defined?

Because the script exits (via the skip/die) before $dir is defined,
and the on-exit (END) handler calls chmod_tree which then uses $dir.


___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils

Re: [PATCH] Again, do not change the mode of all directories below $HOME.

2008-07-22 Thread Jim Meyering 
Ralf Wildenhues [EMAIL PROTECTED] wrote:
 * tests/CuTmpdir.pm (chmod_tree): Do not run chmod on undefined
 argument, can happen when the build path contains spaces.
 ---

 this fixes a regression of make check when source and build tree live
 in a directory with spaces in the name.  The regression was introduced
 some time after I posted a fix for the issue last time.

Thanks, Ralf!

Look at it as an incentive not to use directories with names
containing meta-characters ;-)

But seriously, considering the potential for damage and mischief,
perhaps it's time to add infrastructure that stops e.g., configure
in its tracks whenever it detects a potentially troublesome source or
build dir.

I applied your patch, then added a test to ensure there is no further
regression, in spite of the added cost to make distcheck.  Finally,
I fixed a similar (and long-standing!) shell-under-quoting bug in
test-lib.sh that was exposed by the cleanup (trap-run) code not removing
a per-test directory for the tests that create unsearchable directories.
With a bad build directory containing an a b component, instead of
running e.g., chmod -R 700 /.../a b/..., the pre-patch trap/cleanup
code would run chmod -R 700 /.../a.

Here are the three change sets I've just pushed:

From 1ee81530c09644492d5926b17174140c0a08ad22 Mon Sep 17 00:00:00 2001
From: Ralf Wildenhues [EMAIL PROTECTED]
Date: Tue, 22 Jul 2008 07:38:31 +0200
Subject: [PATCH] tests: again, do not change the mode of all directories below 
$HOME

* tests/CuTmpdir.pm (chmod_tree): Do not run chmod on undefined
argument, can happen when the build path contains spaces.
---
 tests/CuTmpdir.pm |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tests/CuTmpdir.pm b/tests/CuTmpdir.pm
index a7dd8b6..166e50b 100644
--- a/tests/CuTmpdir.pm
+++ b/tests/CuTmpdir.pm
@@ -45,7 +45,7 @@ sub chmod_1

 sub chmod_tree
 {
-  if (chdir $dir)
+  if (defined $dir  chdir $dir)
 {
   # Perform the equivalent of find . -type d -print0|xargs -0 chmod -R 700.
   my $options = {untaint = 1, wanted = \chmod_1};
--
1.6.0.rc0.2.g9b02c


From 9bb0d5766eeb200dae447a616903f14a0079aa63 Mon Sep 17 00:00:00 2001
From: Jim Meyering [EMAIL PROTECTED]
Date: Tue, 22 Jul 2008 09:20:52 +0200
Subject: [PATCH] tests: ensure make check w/tainted build dir no longer 
impacts $HOME

* maint.mk (taint-distcheck): New rule.
(maintainer-distcheck): Make it.
---
 maint.mk |   31 +++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/maint.mk b/maint.mk
index 19b7f12..eb241b6 100644
--- a/maint.mk
+++ b/maint.mk
@@ -667,6 +667,7 @@ cvs-check: vc-diff-check

 maintainer-distcheck:
$(MAKE) distcheck
+   $(MAKE) taint-distcheck
$(MAKE) my-distcheck


@@ -695,6 +696,36 @@ TMPDIR ?= /tmp
 t=$(TMPDIR)/$(PACKAGE)/test
 pfx=$(t)/i

+# More than once, tainted build and source directory names would
+# have caused at least one make check test to apply chmod 700
+# to all directories under $HOME.  Make sure it doesn't happen again.
+tp := $(shell echo $(TMPDIR)/$(PACKAGE)-)
+t_prefix = $(tp)/a
+t_taint = '$(t_prefix) b'
+fake_home = $(tp)/home
+
+# Ensure that tests run from tainted build and src dir names work,
+# and don't affect anything in $HOME.  Create witness files in $HOME,
+# record their attributes, and build/test.  Then ensure that the
+# witnesses were not affected.
+taint-distcheck: $(DIST_ARCHIVES)
+   test -d $(t_taint)  chmod -R 700 $(t_taint) || :
+   -rm -rf $(t_taint) $(fake_home)
+   mkdir -p $(t_prefix) $(t_taint) $(fake_home)
+   GZIP=$(GZIP_ENV) $(AMTAR) -C $(t_taint) -zxf $(distdir).tar.gz
+   mkfifo $(fake_home)/fifo
+   touch $(fake_home)/f
+   mkdir -p $(fake_home)/d/e
+   ls -lR $(fake_home) $(t_prefix)  $(tp)/.ls-before
+   cd $(t_taint)/$(distdir)\
+  ./configure\
+  $(MAKE)\
+  HOME=$(fake_home) $(MAKE) check\
+  ls -lR $(fake_home) $(t_prefix)  $(tp)/.ls-after \
+  diff $(tp)/.ls-before $(tp)/.ls-after  \
+  test -d $(t_prefix)
+   rm -rf $(tp)
+
 # Verify that a twisted use of --program-transform-name=PROGRAM works.
 define install-transform-check
   rm -rf $(pfx);   \
--
1.6.0.rc0.2.g9b02c


From f82c5ba71efd1228a58953b6efc5e0d84c73e8a4 Mon Sep 17 00:00:00 2001
From: Jim Meyering [EMAIL PROTECTED]
Date: Tue, 22 Jul 2008 11:23:08 +0200
Subject: [PATCH] tests: do not run chmod on a prefix of space-embedded tmpdir

* TESTS/test-lib.sh (remove_tmp_): New function.
(trap 0): Use it instead of open-coded (and misquoted) version.
---
 tests/test-lib.sh |   10 --
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/test-lib.sh b/tests/test-lib.sh
index 2083d0c..f386933 100644
--- a/tests/test-lib.sh
+++ b/tests/test-lib.sh
@@ -271,10 +271,16 @@ cleanup_() { :; }

Re: [PATCH] Again, do not change the mode of all directories below $HOME.

2008-07-22 Thread Ralf Wildenhues 
Hi Jim,

* Jim Meyering wrote on Tue, Jul 22, 2008 at 01:17:20PM CEST:
 Ralf Wildenhues [EMAIL PROTECTED] wrote:
  this fixes a regression of make check when source and build tree live
  in a directory with spaces in the name.  The regression was introduced
  some time after I posted a fix for the issue last time.

 Look at it as an incentive not to use directories with names
 containing meta-characters ;-)

Well, *I* am doing it for fun.  Others use /c/My Files/... because yet
others thought requiring users to cope with such names would be a cool
idea.  The slightly provoking subject is to remind you that I suffered
when this happened to me for the first time.  ;-)

 But seriously, considering the potential for damage and mischief,
 perhaps it's time to add infrastructure that stops e.g., configure
 in its tracks whenever it detects a potentially troublesome source or
 build dir.

I disagree.  You can do that and admit defeat.  I worked to mostly[1]
fix Automake for this, because users kept complaining, and now refuse
to admit defeat.

 I applied your patch, then added a test to ensure there is no further
 regression, in spite of the added cost to make distcheck.  Finally,
 I fixed a similar (and long-standing!) shell-under-quoting bug in
 test-lib.sh that was exposed by the cleanup (trap-run) code not removing
 a per-test directory for the tests that create unsearchable directories.
 With a bad build directory containing an a b component, instead of
 running e.g., chmod -R 700 /.../a b/..., the pre-patch trap/cleanup
 code would run chmod -R 700 /.../a.

Would the following not have sufficed, too?

 -trap 'st=$?; cleanup_; d='$t_';
 -cd '$test_dir_'  chmod -R u+rwx $d  rm -rf $d  exit $st' 0
  +trap 'st=$?; cleanup_; d=$t_;
  +cd $test_dir_  chmod -R u+rwx $d  rm -rf $d  exit $st' 0


BTW, I see that you use 'local' in shell functions.  This isn't required
to be supported by POSIX sh, and AFAIK won't work with some ksh variants,
for example the Version M 1993-12-28 r on my GNU/Linux system, but
also your test-lib.sh doesn't check for this capability.  I suggest to
just avoid local variables, since you have only a couple anyway.  Should
I write a patch to this end?

FWIW, your second patch looks like it won't quite do what you intended
if TMPDIR contains white space:

  tp := $(shell echo $(TMPDIR)/$(PACKAGE)-)
  t_prefix = $(tp)/a
  ... using $(t_prefix) unquoted in a number of places ...

Cheers,
Ralf

[1] You currently have to do something like this:
  ./configure -C MISSING=/usr/share/automake-1.10a/missing \
 install_sh=/usr/share/automake-1.10a/install-sh


___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils

Re: [PATCH] Again, do not change the mode of all directories below $HOME.

2008-07-22 Thread Jim Meyering 
Ralf Wildenhues [EMAIL PROTECTED] wrote:
 Look at it as an incentive not to use directories with names
 containing meta-characters ;-)

 Well, *I* am doing it for fun.  Others use /c/My Files/... because yet
 others thought requiring users to cope with such names would be a cool
 idea.  The slightly provoking subject is to remind you that I suffered
 when this happened to me for the first time.  ;-)

 But seriously, considering the potential for damage and mischief,
 perhaps it's time to add infrastructure that stops e.g., configure
 in its tracks whenever it detects a potentially troublesome source or
 build dir.

 I disagree.  You can do that and admit defeat.  I worked to mostly[1]
 fix Automake for this, because users kept complaining, and now refuse
 to admit defeat.

Hi Ralf,

I wouldn't call it admitting defeat.
It's more like admitting that not everyone will go to
the required lengths to make their code robust enough
to deal with such situations.

 I applied your patch, then added a test to ensure there is no further
 regression, in spite of the added cost to make distcheck.  Finally,
 I fixed a similar (and long-standing!) shell-under-quoting bug in
 test-lib.sh that was exposed by the cleanup (trap-run) code not removing
 a per-test directory for the tests that create unsearchable directories.
 With a bad build directory containing an a b component, instead of
 running e.g., chmod -R 700 /.../a b/..., the pre-patch trap/cleanup
 code would run chmod -R 700 /.../a.

 Would the following not have sufficed, too?

 -trap 'st=$?; cleanup_; d='$t_';
 -cd '$test_dir_'  chmod -R u+rwx $d  rm -rf $d  exit $st' 0
   +trap 'st=$?; cleanup_; d=$t_;
   +cd $test_dir_  chmod -R u+rwx $d  rm -rf $d  exit $st' 0

I don't see how that would change anything.  The cd is just to
get out of the directory we're about to remove.  It worked.
The chmod is what failed, operating on the prefix up to,
but not including the space.

 BTW, I see that you use 'local' in shell functions.  This isn't required
 to be supported by POSIX sh, and AFAIK won't work with some ksh variants,
 for example the Version M 1993-12-28 r on my GNU/Linux system, but
 also your test-lib.sh doesn't check for this capability.  I suggest to
 just avoid local variables, since you have only a couple anyway.  Should
 I write a patch to this end?

I'd be more amenable to a patch that makes a macro like
posix-shell.m4 choose a shell with the features I use.
Since no one has reported trouble yet, perhaps it's not a problem
in practice.

 FWIW, your second patch looks like it won't quite do what you intended
 if TMPDIR contains white space:

   tp := $(shell echo $(TMPDIR)/$(PACKAGE)-)
   t_prefix = $(tp)/a
   ... using $(t_prefix) unquoted in a number of places ...

Yes.  That was deliberate.
With those rules, I have never bothered to accommodate white space in TMPDIR.
I expect that anyone clueful enough to run those optional rules also
knows not to choose a TMPDIR value that is likely to cause trouble.

Patches welcome.


___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils