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 <[email protected]>
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.
Miklos Szeredi <[email protected]> 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.
> > 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
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.
> > 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
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...
> > > > 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
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