"Switch !O_CREAT case to use of do_last()" causes segfault in glibc

"Switch !O_CREAT case to use of do_last()" causes segfault in glibc

[walt]

Hi Al,
commit 1f36f774b22a0ceb7dd33eca626746c81a97b6a5
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Sat Dec 26 10:56:19 2009 -0500

     Switch !O_CREAT case to use of do_last()

     ... and now we have all intents crap well localized

This commit is triggering a segfault when I try to open a spreadsheet in
openoffice:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff6d13129 in __readdir (dirp=0x0) at ../sysdeps/unix/readdir.c:45
45      ../sysdeps/unix/readdir.c: No such file or directory.
         in ../sysdeps/unix/readdir.c
(gdb) bt
#0  0x00007ffff6d13129 in __readdir (dirp=0x0) at ../sysdeps/unix/readdir.c:45
#1  0x00007ffff1dc58b0 in ?? () from /usr/lib/libicuuc.so.42
#2  0x00007ffff1dc591d in ?? () from /usr/lib/libicuuc.so.42
#3  0x00007ffff1dc5d7e in uprv_tzname_4_2 () from /usr/lib/libicuuc.so.42
#4  0x00007fffd7f73601 in icu_4_2::TimeZone::initDefault() () from /usr/lib/libicui18n.so.42
#5  0x00007fffd7f738c5 in icu_4_2::TimeZone::createDefault() () from /usr/lib/libicui18n.so.42
#6  0x00007fffd7f6e5c9 in icu_4_2::Calendar::createInstance(icu_4_2::Locale const&, UErrorCode&) ()
    from /usr/lib/libicui18n.so.42


Here is readdir.c:445 (from glibc-2.11):

#ifndef NOT_IN_libc
   __libc_lock_lock (dirp->lock);
#endif

Thanks.

Re: "Switch !O_CREAT case to use of do_last()" causes segfault in glibc

[Al Viro]

On Sat, Mar 06, 2010 at 09:43:42AM -0800, walt wrote:
> Hi Al,
> commit 1f36f774b22a0ceb7dd33eca626746c81a97b6a5
> Author: Al Viro <viro@zeniv.linux.org.uk>
> Date:   Sat Dec 26 10:56:19 2009 -0500
> 
>     Switch !O_CREAT case to use of do_last()
> 
>     ... and now we have all intents crap well localized
> 
> This commit is triggering a segfault when I try to open a spreadsheet in
> openoffice:
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007ffff6d13129 in __readdir (dirp=0x0) at ../sysdeps/unix/readdir.c:45
> 45      ../sysdeps/unix/readdir.c: No such file or directory.
>         in ../sysdeps/unix/readdir.c
> (gdb) bt
> #0  0x00007ffff6d13129 in __readdir (dirp=0x0) at ../sysdeps/unix/readdir.c:45
> #1  0x00007ffff1dc58b0 in ?? () from /usr/lib/libicuuc.so.42
> #2  0x00007ffff1dc591d in ?? () from /usr/lib/libicuuc.so.42
> #3  0x00007ffff1dc5d7e in uprv_tzname_4_2 () from /usr/lib/libicuuc.so.42
> #4  0x00007fffd7f73601 in icu_4_2::TimeZone::initDefault() () from /usr/lib/libicui18n.so.42
> #5  0x00007fffd7f738c5 in icu_4_2::TimeZone::createDefault() () from /usr/lib/libicui18n.so.42
> #6  0x00007fffd7f6e5c9 in icu_4_2::Calendar::createInstance(icu_4_2::Locale const&, UErrorCode&) ()
>    from /usr/lib/libicui18n.so.42

Very interesting.  Could you give the relevant parts of strace (or reproduce
it with something less monumental than openoffice)?

Re: "Switch !O_CREAT case to use of do_last()" causes segfault in glibc

[Al Viro]

On Sat, Mar 06, 2010 at 05:59:18PM +0000, Al Viro wrote:
> On Sat, Mar 06, 2010 at 09:43:42AM -0800, walt wrote:
> > Hi Al,
> > commit 1f36f774b22a0ceb7dd33eca626746c81a97b6a5
> > Author: Al Viro <viro@zeniv.linux.org.uk>
> > Date:   Sat Dec 26 10:56:19 2009 -0500
> > 
> >     Switch !O_CREAT case to use of do_last()
> > 
> >     ... and now we have all intents crap well localized
> > 
> > This commit is triggering a segfault when I try to open a spreadsheet in
> > openoffice:
> > 
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x00007ffff6d13129 in __readdir (dirp=0x0) at ../sysdeps/unix/readdir.c:45
> > 45      ../sysdeps/unix/readdir.c: No such file or directory.
> >         in ../sysdeps/unix/readdir.c
> > (gdb) bt
> > #0  0x00007ffff6d13129 in __readdir (dirp=0x0) at ../sysdeps/unix/readdir.c:45
> > #1  0x00007ffff1dc58b0 in ?? () from /usr/lib/libicuuc.so.42
> > #2  0x00007ffff1dc591d in ?? () from /usr/lib/libicuuc.so.42
> > #3  0x00007ffff1dc5d7e in uprv_tzname_4_2 () from /usr/lib/libicuuc.so.42
> > #4  0x00007fffd7f73601 in icu_4_2::TimeZone::initDefault() () from /usr/lib/libicui18n.so.42
> > #5  0x00007fffd7f738c5 in icu_4_2::TimeZone::createDefault() () from /usr/lib/libicui18n.so.42
> > #6  0x00007fffd7f6e5c9 in icu_4_2::Calendar::createInstance(icu_4_2::Locale const&, UErrorCode&) ()
> >    from /usr/lib/libicui18n.so.42
> 
> Very interesting.  Could you give the relevant parts of strace (or reproduce
> it with something less monumental than openoffice)?

_Really_ interesting; it doesn't look like an oops - smells like an attempt
to do opendir() that fails for some reason, goes unnoticed and resulting
FILE * (i.e. NULL) is fed to readdir()?

What does it attempt to open?

Re: "Switch !O_CREAT case to use of do_last()" causes segfault in glibc

[walt]

On 03/06/2010 10:03 AM, Al Viro wrote:

> _Really_ interesting; it doesn't look like an oops - smells like an attempt
> to do opendir() that fails for some reason, goes unnoticed and resulting
> FILE * (i.e. NULL) is fed to readdir()?
>
> What does it attempt to open?

Ah, this may help:

open("/usr/share/zoneinfo/", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 47
open("/usr/share/zoneinfo/MST7MDT", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 48
open("/usr/share/zoneinfo/MST7MDT/", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = -1 ENOTDIR (Not a directory)
--- SIGSEGV (Segmentation fault) @ 0 (0) ---

Re: "Switch !O_CREAT case to use of do_last()" causes segfault in glibc

[Al Viro]

On Sat, Mar 06, 2010 at 10:20:41AM -0800, walt wrote:
> On 03/06/2010 10:03 AM, Al Viro wrote:
> 
> >_Really_ interesting; it doesn't look like an oops - smells like an attempt
> >to do opendir() that fails for some reason, goes unnoticed and resulting
> >FILE * (i.e. NULL) is fed to readdir()?
> >
> >What does it attempt to open?
> 
> Ah, this may help:
> 
> open("/usr/share/zoneinfo/", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 47
> open("/usr/share/zoneinfo/MST7MDT", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 48
> open("/usr/share/zoneinfo/MST7MDT/", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = -1 ENOTDIR (Not a directory)
> --- SIGSEGV (Segmentation fault) @ 0 (0) ---

Oho...  And what do you have in /usr/share/zoneinfo?  I.e. what is MST7MDT?

BTW, icu is buggy, regardless of apparent bug introduced in the kernel -
common/putil.c:searchForTZFile() does

    DIR* dirp = opendir(path);
...
    while((dirEntry = readdir(dirp)) != NULL) {

with no references to dirp in between (no checks for errors, etc.), so it'll
segfault on any number of situations (e.g. directory in question being
unreadable for you).

Re: "Switch !O_CREAT case to use of do_last()" causes segfault in glibc

[Al Viro]

On Sat, Mar 06, 2010 at 10:20:41AM -0800, walt wrote:
> On 03/06/2010 10:03 AM, Al Viro wrote:
> 
> >_Really_ interesting; it doesn't look like an oops - smells like an attempt
> >to do opendir() that fails for some reason, goes unnoticed and resulting
> >FILE * (i.e. NULL) is fed to readdir()?
> >
> >What does it attempt to open?
> 
> Ah, this may help:
> 
> open("/usr/share/zoneinfo/", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 47
> open("/usr/share/zoneinfo/MST7MDT", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 48

Now that is interesting.  Looks like it has managed to lose O_DIRECTORY check
in the open without trailing slash and (properly) barfed with / added.

Re: "Switch !O_CREAT case to use of do_last()" causes segfault in glibc

[Al Viro]

On Sat, Mar 06, 2010 at 06:32:54PM +0000, Al Viro wrote:
> On Sat, Mar 06, 2010 at 10:20:41AM -0800, walt wrote:
> > On 03/06/2010 10:03 AM, Al Viro wrote:
> > 
> > >_Really_ interesting; it doesn't look like an oops - smells like an attempt
> > >to do opendir() that fails for some reason, goes unnoticed and resulting
> > >FILE * (i.e. NULL) is fed to readdir()?
> > >
> > >What does it attempt to open?
> > 
> > Ah, this may help:
> > 
> > open("/usr/share/zoneinfo/", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 47
> > open("/usr/share/zoneinfo/MST7MDT", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 48
> 
> Now that is interesting.  Looks like it has managed to lose O_DIRECTORY check
> in the open without trailing slash and (properly) barfed with / added.

OK, I can reproduce it.  Give me a few, I'll see...

AAARGH.

Fix a dumb typo - use of & instead of &&

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---

diff --git a/fs/namei.c b/fs/namei.c
index 3d9d2f9..48e60a1 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1656,7 +1656,7 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 		if (path->dentry->d_inode->i_op->follow_link)
 			return NULL;
 		error = -ENOTDIR;
-		if (*want_dir & !path->dentry->d_inode->i_op->lookup)
+		if (*want_dir && !path->dentry->d_inode->i_op->lookup)
 			goto exit_dput;
 		path_to_nameidata(path, nd);
 		audit_inode(pathname, nd->path.dentry);