O_DIRECTORY|O_CREAT handling

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

The kernel currently handles open() calls with flags having at least O_DIRECTORY|O_CREAT set strangely (to say the least). It creates a regular file, completely ignoring the O_DIRECTORY bit. One can argue that open() does not perform any real checking on the parameters and that it is the programmers responsibility, but there is a twist.

Some programs create temporary directory which they afterward use as the working directory or changed root. I.e., we have code like this:

  mkdir ("/some/dirRANDOM");
  chdir ("/some/dirRANDOM");

or

  mkdir ("/some/dirRANDOM");
  fd = open ("/some/dirRANDOM", O_DIRECTORY);
  fchdir (fd);

or

  mkdir ("/some/dirRANDOM");
  chroot ("/some/dirRANDOM");

All these pieces of code have an obvious flaw, a race. There is no atomic way to do what we want.

Now combine these two problems. How about making this work?

  fd = open ("/some/dirRANDOM", O_RDONLY|O_CREAT|O_DIRECTORY|O_EXCL, 0700);
  fchdir (fd);

(and similarly for a new interface fchroot which I can add to glibc).

I've attached a little patch which does just that. Some comments on the code:

~ if an existing directory is opened with O_RDWR open() returns
- -EISDIR. I've mimicked this, allthough the error code might not be the most obvious.

~ O_TRUNC is not allowed.

The small attached patch implements this proposal. At least it does what I want it to do, no idea what bugs I introduce.

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖ -----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (GNU/Linux)

iD8DBQE/5WqW2ijCOnn/RHQRAjZEAJ9SxzM1O6B/hQZN5jabqSSzXXtZwQCdH2sd hRr+ejRNAU4Cl9V8MXpBIYo=
=jXqY
-----END PGP SIGNATURE-----

@@ -1295,7 +1295,12 @@
 	if (!dentry->d_inode) {
 		if (!IS_POSIXACL(dir->d_inode))
 			mode &= ~current->fs->umask;
-		error = vfs_create(dir->d_inode, dentry, mode, nd);
+		if (unlikely(flag & O_DIRECTORY)) {
+			error = -EISDIR;
+			if (!(flag & FMODE_WRITE))
+				error = vfs_mkdir(dir->d_inode, dentry, mode);
+		} else
+			error = vfs_create(dir->d_inode, dentry, mode, nd);
 		up(&dir->d_inode->i_sem);
 		dput(nd->dentry);
 		nd->dentry = dentry;

@@ -1331,7 +1336,11 @@
 	dput(nd->dentry);
 	nd->dentry = dentry;
 	error = -EISDIR;
-	if (dentry->d_inode && S_ISDIR(dentry->d_inode->i_mode))
+	/* Since we allow creating directories with open(2) we forbid
+	   opening an existing directory with O_CREAT only if the
+	   O_DIRECTORY flag is not set or if truncation is requested.  */
+	if (dentry->d_inode && S_ISDIR(dentry->d_inode->i_mode) &&
+	    (!(flag & O_DIRECTORY) || (flag & O_TRUNC)))
 		goto exit;
 ok:
 	error = may_open(nd, acc_mode, flag);

Hi Ulrich,

Although I agree on the race, I fail to see in what case it matters. In my programs, I often use mkdir() to get temporary directories instead of temporary files, just because of the atomicity of the test-and-set which mkdir() provides. Basically, I do :

  base_dir="/tmp/tmpdir"; 
  do {
     rnd=random();
     sprintf(dir, "%s%d", base_dir, rnd);
  } while (!mkdir(dir, 0700);
  /* now I'm guaranteed that I'm the first to get this dir, */
  /* and only my UID can work in it */
  chdir(dir);

So the only race would be someone working with the same UID (or root) removing the directory and replacing it with another one (or a symlink or anything) between mkdir() and chdir(). But don't see any use or consequence to this.

It would be interesting, of course, but is it portable to other systems ? If it is not, very few people will use it, unfortunately. But if others already do it right, then why not include it ?

Cheers,
Willy

If /tmp isn't sticky, anyone can do this. I think this is the scenario Ulrich was referring to.

Andrew

And if /etc/shadow is world-writable, anyone can do other interesting things. Doctor, it hurts when I do it...