bug-coreutils
[Top][All Lists]
Advanced

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

Re: minor mkdir bug fixes


From: Jim Meyering
Subject: Re: minor mkdir bug fixes
Date: Wed, 15 Jun 2005 11:45:03 +0200

Thanks for all of your cleanup work and for catching the true/false bug!

Paul Eggert <address@hidden> wrote:
> I found some more glitches in the mkdir code.  Occasionally multiple
> diagnostics might be generated where one would do.  The diagnostics

I spotted another problem.
install -d could still create directories in the wrong place, albeit only
under contrived conditions.  That was due to a typo in one of my changes.
I've just fixed that and added to the existing tests/install/basic-1
test script so it would detect the resulting failure.

> could report the error number, but didn't.  In one unlikely case in
> 'install', the code would install the file into the wrong place.

If the bug is not too hard to test for, would you please create a
test case for it, so it'll be less likely to be reintroduced later?
These corner case bugs are hard enough to find.
If we don't record/document them via test cases,
we can't even hope to converge on correctness.

> Also, there's no need for make_dir_parents to invoke 'stat' each time
> through the loop.  It can just rely on information gleaned from the
> other system calls.  In theory even the first "stat" could go but I'm
> not sure that would improve performance on the average, since "mkdir
> -p /some/existing/file" must be a common case.

Finally, there was a new small bug in mkdir-p.c.
Patch and test case below.

2005-06-15  Jim Meyering  <address@hidden>

        * lib/mkdir-p.c (make_dir_parents): Don't let a failed chdir($PWD)
        stop us from restricting permissions of just-created absolute-named
        directories.
        * tests/mkdir/p-3: Add a test for just-fixed bug in mkdir-p.c.

        * src/install.c (main): Fix my typo: s/argv[optind]/file[i]/.
        * tests/install/basic-1: Ensure that each `-d'-specified directory
        is created.  Ensure that rel-named dirs are not created when
        chdir($PWD) fails.

Index: lib/mkdir-p.c
===================================================================
RCS file: /fetish/cu/lib/mkdir-p.c,v
retrieving revision 1.7
retrieving revision 1.8
diff -u -p -u -r1.7 -r1.8
--- lib/mkdir-p.c       14 Jun 2005 23:56:49 -0000      1.7
+++ lib/mkdir-p.c       15 Jun 2005 08:31:44 -0000      1.8
@@ -319,7 +319,8 @@ make_dir_parents (char const *arg,
   for (; leading_dirs != NULL; leading_dirs = leading_dirs->next)
     {
       leading_dirs->dirname_end[0] = '\0';
-      if (cwd_problem || chmod (full_dir, parent_mode) != 0)
+      if ((cwd_problem && *full_dir != '/')
+         || chmod (full_dir, parent_mode) != 0)
        {
          error (0, (cwd_problem ? 0 : errno),
                 _("cannot change permissions of %s"), quote (full_dir));

Index: tests/mkdir/p-3
===================================================================
RCS file: /fetish/cu/tests/mkdir/p-3,v
retrieving revision 1.3
retrieving revision 1.4
diff -u -p -u -r1.3 -r1.4
--- tests/mkdir/p-3     14 Jun 2005 09:00:00 -0000      1.3
+++ tests/mkdir/p-3     15 Jun 2005 08:32:11 -0000      1.4
@@ -18,6 +18,7 @@ mkdir -p $tmp || framework_failure=1
 cd $tmp || framework_failure=1
 mkdir no-access || framework_failure=1
 mkdir no-acce2s || framework_failure=1
+mkdir no-acce3s || framework_failure=1
 
 if test $framework_failure = 1; then
   echo "$0: failure in testing framework" 1>&2
@@ -26,16 +27,25 @@ fi
 
 p=$pwd/$tmp
 (cd no-access; chmod 0 . && mkdir -p $p/a/b u/v) 2> /dev/null && fail=1
+test -d $p/a/b || fail=1
 
 # Same as above, but with a following *absolute* name, it should succeed
-(cd no-acce2s; chmod 0 . && mkdir -p $p/a/b $p/z) || fail=1
-
-test -d $p/a/b || fail=1
-b=`ls $p/a|tr -d '\n'`
+(cd no-acce2s; chmod 0 . && mkdir -p $p/b/b $p/z) || fail=1
 
 test -d $p/z || fail=1
 
+b=`ls $p/a|tr -d '\n'`
 # With coreutils-5.3.0, this would fail with $b=bu.
 test "x$b" = xb || fail=1
 
+# Ensure that the re_protect code is run on absolute names, even
+# after failure to return to the initial working directory.
+# This is actually a test of the underlying mkdir-p.c code.
+# The part in question cannot be tested via mkdir(1) because that
+# program cannot create leading directories that lack u=wx permissions,
+# so we have to test with install (aka ginstall in the build directory).
+(cd no-acce3s; chmod 0 . && ginstall -m 0 -d $p/c/b $p/y/z) || fail=1
+p=`ls -ld $p/y|sed 's/ .*//'`
+case $p in d---------);; *) fail=1;; esac
+
 exit $fail
Index: src/install.c
===================================================================
RCS file: /fetish/cu/src/install.c,v
retrieving revision 1.184
retrieving revision 1.185
diff -u -p -u -r1.184 -r1.185
--- src/install.c       14 Jun 2005 23:55:24 -0000      1.184
+++ src/install.c       15 Jun 2005 08:54:21 -0000      1.185
@@ -360,7 +360,7 @@ main (int argc, char **argv)
       int cwd_errno = 0;
       for (i = 0; i < n_files; i++)
        {
-         if (cwd_errno != 0 && IS_RELATIVE_FILE_NAME (argv[optind]))
+         if (cwd_errno != 0 && IS_RELATIVE_FILE_NAME (file[i]))
            {
              error (0, cwd_errno, _("cannot return to working directory"));
              ok = false;

              
Index: tests/install/basic-1
===================================================================
RCS file: /fetish/cu/tests/install/basic-1,v
retrieving revision 1.15
retrieving revision 1.17
diff -u -p -r1.15 -r1.17
--- tests/install/basic-1       21 Apr 2005 00:30:13 -0000      1.15
+++ tests/install/basic-1       15 Jun 2005 09:31:16 -0000      1.17
@@ -9,12 +9,12 @@ dir=dir
 file=file
 
 pwd=`pwd`
-tmp=inst-basic.$$
-trap 'status=$?; cd $pwd; rm -rf $tmp && exit $status' 0
-trap 'exit $?' 1 2 13 15
+t0=`echo "$0"|sed 's,.*/,,'`.tmp; tmp=$t0/$$
+trap 'status=$?; cd $pwd; chmod -R u+rwx $t0; rm -rf $t0 && exit $status' 0
+trap '(exit $?); exit $?' 1 2 13 15
 
 framework_failure=0
-mkdir $tmp || framework_failure=1
+mkdir -p $tmp || framework_failure=1
 cd $tmp || framework_failure=1
 
 rm -rf $dir $file || framework_failure=1
@@ -36,7 +36,7 @@ test -f $dir/$file || fail=1
 # Make sure strip works.
 dd=dd$EXEEXT
 dd2=dd2$EXEEXT
-cp ../../../src/$dd . || fail=1
+cp $pwd/../../src/$dd . || fail=1
 cp $dd $dd2 || fail=1
 
 strip $dd2 || \
@@ -63,6 +63,26 @@ test "$1" = -r-xr-xr-x || fail=1
 # These failed in coreutils CVS from 2004-06-25 to 2004-08-11.
 ginstall -d . || fail=1
 ginstall -d newdir || fail=1
+test -d newdir || fail=1
 ginstall -d newdir1 newdir2 newdir3 || fail=1
+test -d newdir1 || fail=1
+test -d newdir2 || fail=1
+test -d newdir3 || fail=1
+
+# This fails because mkdir-p.c's make_dir_parents fails to return to its
+# initial working directory ($abs) after creating the first argument, and
+# hence cannot do anything meaningful with the following relative-named dirs.
+abs=$pwd/$tmp
+mkdir sub && cd sub
+chmod 0 .; ginstall -d $abs/xx/yy rel/sub1 rel/sub2 2> /dev/null && fail=1
+chmod 755 $abs/sub
+
+# Ensure that the first argument-dir has been created.
+test -d $abs/xx/yy || fail=1
+
+# Make sure that the `rel' directory was not created...
+test -d $abs/sub/rel && fail=1
+# and make sure it was not created in the wrong place.
+test -d $abs/xx/rel && fail=1
 
 (exit $fail); exit $fail




reply via email to

[Prev in Thread] Current Thread [Next in Thread]