[PATCH] open: O_DIRECTORY and O_CREAT together should fail

[PATCH] open: O_DIRECTORY and O_CREAT together should fail

[Miklos Szeredi]

Add a check for O_DIRECTORY in the O_CREAT path, and return -EINVAL.

Current behavior is inconsistent with documentation: 
open(..., O_DIRECTORY|O_CREAT) succeeds if file didn't exist, and
returned descriptor does not refer to a directory.

No other error value quite fits this case: 

  ENOTDIR - the file doesn't exist, so this is slightly misleading
  ENOENT - yes, but we asked for an O_CREAT so why wasn't it created

But EINVAL - invalid combination of flags, is quite good IMO.

Signed-off-by: Miklos Szeredi <miklos@szeredi.hu>

Index: linux/fs/namei.c
===================================================================
--- linux.orig/fs/namei.c	2005-09-23 16:35:32.000000000 +0200
+++ linux/fs/namei.c	2005-09-23 16:36:19.000000000 +0200
@@ -1441,6 +1441,9 @@ int open_namei(const char * pathname, in
 			return error;
 		goto ok;
 	}
+	/* O_CREAT | O_DIRECTORY should fail */
+	if (flag & O_DIRECTORY)
+		return -EINVAL;
 
 	/*
 	 * Create - we need to know the parent.

Re: [PATCH] open: O_DIRECTORY and O_CREAT together should fail

[Andrew Morton]

Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> Add a check for O_DIRECTORY in the O_CREAT path, and return -EINVAL.
> 
> Current behavior is inconsistent with documentation: 
> open(..., O_DIRECTORY|O_CREAT) succeeds if file didn't exist, and
> returned descriptor does not refer to a directory.
> 
> No other error value quite fits this case: 
> 
>   ENOTDIR - the file doesn't exist, so this is slightly misleading
>   ENOENT - yes, but we asked for an O_CREAT so why wasn't it created
> 
> But EINVAL - invalid combination of flags, is quite good IMO.
> 

We could be a bit screwed here.  If there are any apps out there which are
using this combination, we just broke them.  Essentially the patch is
assuming that nobody is currently using O_CREAT|O_DIRECTORY, but one day in
the future someone will do that.


> 
> Index: linux/fs/namei.c
> ===================================================================
> --- linux.orig/fs/namei.c	2005-09-23 16:35:32.000000000 +0200
> +++ linux/fs/namei.c	2005-09-23 16:36:19.000000000 +0200
> @@ -1441,6 +1441,9 @@ int open_namei(const char * pathname, in
>  			return error;
>  		goto ok;
>  	}
> +	/* O_CREAT | O_DIRECTORY should fail */
> +	if (flag & O_DIRECTORY)
> +		return -EINVAL;
>  
>  	/*
>  	 * Create - we need to know the parent.

Re: [PATCH] open: O_DIRECTORY and O_CREAT together should fail

[Miklos Szeredi]

> > Current behavior is inconsistent with documentation: 
> > open(..., O_DIRECTORY|O_CREAT) succeeds if file didn't exist, and
> > returned descriptor does not refer to a directory.
> > 
> > No other error value quite fits this case: 
> > 
> >   ENOTDIR - the file doesn't exist, so this is slightly misleading
> >   ENOENT - yes, but we asked for an O_CREAT so why wasn't it created
> > 
> > But EINVAL - invalid combination of flags, is quite good IMO.
> > 
> 
> We could be a bit screwed here.  If there are any apps out there which are
> using this combination, we just broke them.  Essentially the patch is
> assuming that nobody is currently using O_CREAT|O_DIRECTORY, but one day in
> the future someone will do that.

Well yes.  But I don't think anybody is using it, and if so they are
clearly breaking the rules in man open(2):

       O_DIRECTORY
              If pathname is not a directory, cause the open  to  fail.   This
              flag is Linux‐specific, and was added in kernel version 2.1.126,
              to avoid denial‐of‐service problems if opendir(3) is called on a
              FIFO  or  tape  device,  but  should  not be used outside of the
              implementation of opendir.

So if someone uses this outside of opendir() and uses it to create a
non-directory, I think they deserve to be screwed.

Miklos

Re: [PATCH] open: O_DIRECTORY and O_CREAT together should fail

[Al Viro]

On Sat, Sep 24, 2005 at 07:52:06AM +0200, Miklos Szeredi wrote:
> Well yes.  But I don't think anybody is using it, and if so they are
> clearly breaking the rules in man open(2):

Be liberal in what you accept and all such...  Everything else aside,
why bother?  This check doesn't buy you anything.

Re: [PATCH] open: O_DIRECTORY and O_CREAT together should fail

[Miklos Szeredi]

> > Well yes.  But I don't think anybody is using it, and if so they are
> > clearly breaking the rules in man open(2):
> 
> Be liberal in what you accept and all such...  Everything else aside,
> why bother?

To conform to well defined semantics?

It just bathers me, that you can get a non-directory file descriptor
with O_DIRECTORY.

Miklos

Re: [PATCH] open: O_DIRECTORY and O_CREAT together should fail

[Al Viro]

On Sat, Sep 24, 2005 at 08:41:05AM +0200, Miklos Szeredi wrote:
> > > Well yes.  But I don't think anybody is using it, and if so they are
> > > clearly breaking the rules in man open(2):
> > 
> > Be liberal in what you accept and all such...  Everything else aside,
> > why bother?
> 
> To conform to well defined semantics?

Well-defined is not exactly the word I'd use for that mess (example -
we still have the last remnant of ancient BSD idiocy in there; the last
case when dangling symlink is still traversed upon object creation,
everything else had been fixed since then).

And O_DIRECTORY is not the only flag that acquires or loses meaning
depending on O_CREAT - consider e.g. O_EXCL.  It's a mess, of course,
but this mess is part of userland ABI.  We tried to fix symlink idiocy,
BTW, on the assumption that nothing would be relying on it.  Didn't
work...

Re: [PATCH] open: O_DIRECTORY and O_CREAT together should fail

[Miklos Szeredi]

> > > > Well yes.  But I don't think anybody is using it, and if so they are
> > > > clearly breaking the rules in man open(2):
> > > 
> > > Be liberal in what you accept and all such...  Everything else aside,
> > > why bother?
> > 
> > To conform to well defined semantics?
> 
> Well-defined is not exactly the word I'd use for that mess (example -
> we still have the last remnant of ancient BSD idiocy in there; the last
> case when dangling symlink is still traversed upon object creation,
> everything else had been fixed since then).
> 
> And O_DIRECTORY is not the only flag that acquires or loses meaning
> depending on O_CREAT - consider e.g. O_EXCL.  It's a mess, of course,
> but this mess is part of userland ABI.  We tried to fix symlink idiocy,
> BTW, on the assumption that nothing would be relying on it.  Didn't
> work...

OK, I'm convinced.

Miklos

Re: [PATCH] open: O_DIRECTORY and O_CREAT together should fail

[Kyle Moffett]

On Sep 24, 2005, at 03:01:50, Al Viro wrote:
> And O_DIRECTORY is not the only flag that acquires or loses meaning  
> depending on O_CREAT - consider e.g. O_EXCL.  It's a mess, of  
> course, but this mess is part of userland ABI.  We tried to fix  
> symlink idiocy, BTW, on the assumption that nothing would be  
> relying on it.  Didn't work...

Maybe CONFIG_FIX_CRAPPY_ABI_CORNER_CASES?  If the user is willing to  
deal with some minimal breakage and fix programs relying on icky  
unsupported behavior, then they could turn that on for a slightly  
more secure system.  Make it depend on "experimental" and give it big  
warning messages.  It's likely that some of the more-secure server- 
oriented distros that run patched gcc and such to avoid buffer  
overflow and such might turn it on.

Cheers,
Kyle Moffett

--
Simple things should be simple and complex things should be possible
   -- Alan Kay