Still a problem with autofs-5.0.2-27

Still a problem with autofs-5.0.3-15

would a leaked file descriptor result in a 'read' denial, or simply a 'getattr'?

(In reply to comment #3)
> would a leaked file descriptor result in a 'read' denial, or simply a 'getattr'?
> 

Depends on how the file is opened.  You'll see "write" if the file is opened for
writing.  And it's not when an actual read or write is done, but when the
process is started I believe and the kernel checks the permissions for any open
files.  One trick is to open files with O_CLOEXEC so they get closed
automatically on exec.

The current rawhide glibc and kernel now have the appropriate support to fix the
problem.  Change all socket calls from something like this

   fd = socket(PF_INET, SOCK_STREAM, 0);
   if (fd >= 0)
     fcntl(fd, F_SETFD, FD_CLOEXEC);

to something like this

#ifdef SOCK_CLOEXEC
  fd = socket(PF_INET, SOCK_STREAM|SOCK_CLOEXEC, 0);
  if (fd < 0 && errno == EINVAL)
#endif
    {
      fd = socket(PF_INET, SOCK_STREAM, 0);
      if (fd >= 0)
        fcntl(fd, F_SETFD, FD_CLOEXEC);
    }

This new functionality sets the close-on-exec flags atomically.  There is no way
for the file descriptor to leak and this error will go away.

If needed, you can also set the O_NONBLOCK flag at the same time.  Just add
|SOCK_NONBLOCK to the call as well.

If you have to use accept, pipe, or some other file descriptor producing
interface, they all have variants which allow setting FD_CLOEXEC, too.

*** Bug 466746 has been marked as a duplicate of this bug. ***

This message is a reminder that Fedora 8 is nearing its end of life.
Approximately 30 (thirty) days from now Fedora will stop maintaining
and issuing updates for Fedora 8.  It is Fedora's policy to close all
bug reports from releases that are no longer maintained.  At that time
this bug will be closed as WONTFIX if it remains open with a Fedora 
'version' of '8'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version prior to Fedora 8's end of life.

Bug Reporter: Thank you for reporting this issue and we are sorry that 
we may not be able to fix it before Fedora 8 is end of life.  If you 
would still like to see this bug fixed and are able to reproduce it 
against a later version of Fedora please change the 'version' of this 
bug to the applicable version.  If you are unable to change the version, 
please add a comment here and someone will do it for you.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events.  Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

The process we are following is described here: 
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Created attachment 328438 [details]
Patch to use CLOEXEC flag for open(), pipe() and socket() if possible

Ulrich, could you have a quick look at this patch please.
Are the conditional compilation macros correct?

(In reply to comment #8)
> Ulrich, could you have a quick look at this patch please.
> Are the conditional compilation macros correct?

I assume you checked that you caught all cases.  it's fine to use wrapper functions.  The implementation of the wrapper function is not quite right, though.  The kernel unfortunately doesn't return an error if an unknown flag is passed.

For the open functions you'll have to use something like this:

static int cloexec_works;

static inline int open_fd(const char *path, int flags)
{
	int fd;

#if defined(O_CLOEXEC) && defined(SOCK_CLOEXEC)
	flags |= O_CLOEXEC;
#endif
	fd = open(path, flags);
	if (fd == -1)
		return -1;
	if (cloexec_works == 0) {
		int fl = fcntl(fd, F_GETFD);
		cloexec_works = (fl & FD_CLOEXEC) ? 1 : -1;
	}
	if (cloexec_work > 0)
		return fd;
	fcntl(fd, F_SETFD, FD_CLOEXEC);
	return fd;
}


The pipe and socket code is fine, you'll get error codes there.  But you might want to avoid using the new functionality if after the first call you know that the kernel is too old.  See the cloexec_works variable above.

For the fopen() function, add "e" to the option string.  Then, again, check whether the FD_CLOEXEC flag worked.

(In reply to comment #9)
> (In reply to comment #8)
> > Ulrich, could you have a quick look at this patch please.
> > Are the conditional compilation macros correct?
> 
> I assume you checked that you caught all cases.  it's fine to use wrapper

Yeah, think so, for the ones that matter.
I'll be checking again anyway.

> functions.  The implementation of the wrapper function is not quite right,
> though.  The kernel unfortunately doesn't return an error if an unknown flag is
> passed.
> 
> For the open functions you'll have to use something like this:
> 
> static int cloexec_works;

Right, sounds good, I'll fix that.

...

> The pipe and socket code is fine, you'll get error codes there.  But you might
> want to avoid using the new functionality if after the first call you know that
> the kernel is too old.  See the cloexec_works variable above.

Yep.

> 
> For the fopen() function, add "e" to the option string.  Then, again, check
> whether the FD_CLOEXEC flag worked.

Cool, I didn't see the "e" flag when I was looking around.

Thanks for the help.
Ian

Created attachment 328842 [details]
Updated patch to use CLOEXEC flag for open(), pipe() and socket() if possible

Is there a version for F10 with this patch available for testing?

(In reply to comment #12)
> Is there a version for F10 with this patch available for testing?

Only in Rawhide so far but it would be good to test on F-10.
I'll add the patch later today. I don't have an F-10 install
so I will need to ask you to check a couple of things to make
sure the new CLOEXEC feature is available with F-10.

*** Bug 443321 has been marked as a duplicate of this bug. ***

I have built autofs-5.0.3-38 for F-10 and requested it be
pushed to testing. Can you try it out please.

Ulrich, was this included in F-10?
Ian

(In reply to comment #15)
> Ulrich, was this included in F-10?

All of the functions to support O_CLOEXEC except accept4() are supported in the kernel and glibc in F10.

Well, doesn't seem to help.  Running 5.0.3-38.fc10 and still seeing:

Feb  4 01:50:12 orca kernel: type=1400 audit(1233737412.810:39): avc:  denied  { read } for  pid=23237 comm="umount" path="/proc/2169/mounts" dev=proc ino=71812 scontext=system_u:system_r:mount_t:s0 tcontext=system_u:system_r:automount_t:s0 tclass=file

root      2169     1  0 Feb03 ?        00:00:02 automount

Adding Dan Walsh to see if he has any insight.

What is /proc/*/mounts?  Why would one process try to read another ones?

(In reply to comment #17)
> Feb  4 01:50:12 orca kernel: type=1400 audit(1233737412.810:39): avc:  denied 
> { read } for  pid=23237 comm="umount" path="/proc/2169/mounts" dev=proc

This only means that Ian overlooked an open() call.  It should be easy to locate, giving that it's the special mounts file.

Ah, probably more to do with the use of setmntent/getmntent/endmntent.

(In reply to comment #19)
> Ah, probably more to do with the use of setmntent/getmntent/endmntent.

This is indeed a possibility.

I should just change glibc to always set the close-on-exec flag.  The FILE stream is supposed to be used only for the *mntent functions and a stream cannot be used across exec.

Actually, no, just add "e" to the mode parameter for setmntent().  glibc already handles the rest correctly.

Created attachment 330940 [details]
setmntent patch

This look right?

Or does it also need to be wrapped with

#if defined(O_CLOEXEC) && defined(SOCK_CLOEXEC)
        if (cloexec_works != -1) {

like fopen().

(In reply to comment #23)
> Or does it also need to be wrapped with
> 
> #if defined(O_CLOEXEC) && defined(SOCK_CLOEXEC)
>         if (cloexec_works != -1) {
> 
> like fopen().

setmtnent() has to be handled exactly like fopen().  That's what's used behind the scene.

(In reply to comment #21)
> Actually, no, just add "e" to the mode parameter for setmntent().  glibc
> already handles the rest correctly.

What will happen if this is used on older versions of glibc
that don't understand the "e" option.

IOW, do I need to wrap this so it will be version independent
as this patch will be committed upstream once we sort it out.

Ian

(In reply to comment #18)
> (In reply to comment #17)
> > Feb  4 01:50:12 orca kernel: type=1400 audit(1233737412.810:39): avc:  denied 
> > { read } for  pid=23237 comm="umount" path="/proc/2169/mounts" dev=proc
> 
> This only means that Ian overlooked an open() call.  It should be easy to
> locate, giving that it's the special mounts file.

Hahaha, true, I'm Unix challenged and can't even use a simple
find/grep combination, ;)

(In reply to comment #25)
> (In reply to comment #21)
> > Actually, no, just add "e" to the mode parameter for setmntent().  glibc
> > already handles the rest correctly.
> 
> What will happen if this is used on older versions of glibc
> that don't understand the "e" option.
> 
> IOW, do I need to wrap this so it will be version independent
> as this patch will be committed upstream once we sort it out.

Oh, hang on, you already say it needs to be done the same way
in as fopen() comment #24. Will do.

Ian

Created attachment 330967 [details]
Patch to make setmntent(3) use CLOEXEC feature if possible

Built autofs-5.0.3-39 with patch in comment #28 and requested
push to testing. Can we test this please.

autofs-5.0.3-41 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.