Mkdir slightly broken

Doug Graham dgraham at nortel.com
Wed Aug 13 15:57:43 UTC 2008


In make_directory.c, bb_make_directory() is slightly broken in two ways.
This function messes around with mode and umask, I think in an attempt
to make sure that intermediate directories created with the -p option
are always writable.  However, the umask is only changed if the mode
passed in is not -1, which is the usual case.  So this breaks:

  $ umask 0222
  $ ./busybox mkdir -p foo/bar
  mkdir: cannot create directory 'foo/bar': Permission denied

I think that bb_make_directory() needs to temporarily change the umask
regardless of whether or not the mode passed in is -1.

The second bug is that a chmod() is always done on the last component
of the directory being created:

     if ((mode != -1) && (chmod(path, mode) < 0)){
           fail_msg = "set permissions of";
            break;
     }

Mode can never be -1 at this point, because if it were -1 on entry to
bb_make_directory(), it would have been changed to (0777 & ~mask) at
the top of the function.  So the chmod is always done, and will always
clear any bits other than the 0777 bits.  It our case, we want the new
directory to inherit the setgid bit from the parent directory, but this
chmod breaks that.

Here is a patch for both problems.  With this patch, the umask is *always*
set so that intermediate directories will be writable, but the original
umask is restored before creating the final directory.  So no chmod()
is required in the default case.

*** make_directory.c.orig	Wed Jun 25 08:51:24 2008
--- make_directory.c	Wed Aug 13 11:48:01 2008
***************
*** 35,48 ****
  	struct stat st;
  
  	mask = umask(0);
! 	if (mode == -1) {
! 		umask(mask);
! 		mode = (S_IXUSR | S_IXGRP | S_IXOTH |
! 				S_IWUSR | S_IWGRP | S_IWOTH |
! 				S_IRUSR | S_IRGRP | S_IROTH) & ~mask;
! 	} else {
! 		umask(mask & ~0300);
! 	}
  
  	do {
  		c = 0;
--- 35,41 ----
  	struct stat st;
  
  	mask = umask(0);
!         umask(mask & ~0300);    /* ensure intermediate dirs are u rx */
  
  	do {
  		c = 0;
***************
*** 62,67 ****
--- 55,63 ----
  			}
  		}
  
+                 if (!c)         /* last component use orig umask */
+                         umask(mask);
+ 
  		if (mkdir(path, 0777) < 0) {
  			/* If we failed for any other reason than the directory
  			 * already exists, output a diagnostic and return -1.*/
***************
*** 85,91 ****
  			/* Done.  If necessary, updated perms on the newly
  			 * created directory.  Failure to update here _is_
  			 * an error.*/
- 			umask(mask);
  			if ((mode != -1) && (chmod(path, mode) < 0)){
  				fail_msg = "set permissions of";
  				break;
--- 81,86 ----

--Doug.



More information about the busybox mailing list