Bug 46425 - Apr should set FD_CLOEXEC if APR_FOPEN_NOCLEANUP is not set
Apr should set FD_CLOEXEC if APR_FOPEN_NOCLEANUP is not set
Status: RESOLVED FIXED
Product: APR
Classification: Unclassified
Component: APR
HEAD
Other Linux
: P2 normal (vote)
: ---
Assigned To: Apache Portable Runtime bugs mailinglist
:
Depends on:
Blocks: 43965
  Show dependency tree
 
Reported: 2008-12-20 14:59 UTC by Stefan Fritsch
Modified: 2014-02-17 13:51 UTC (History)
2 users (show)



Attachments
Patch for trunk to set FD_CLOEXEC (6.24 KB, patch)
2008-12-21 06:04 UTC, Stefan Fritsch
Details | Diff
updated patch that uses secure way of descriptors handling (see drepper note) where possible (8.14 KB, patch)
2009-02-21 15:33 UTC, Arkadiusz Miskiewicz
Details | Diff
updated patch that uses secure way of descriptors handling (8.15 KB, patch)
2009-02-21 16:10 UTC, Arkadiusz Miskiewicz
Details | Diff
updated patch (18.08 KB, patch)
2009-02-22 05:06 UTC, Stefan Fritsch
Details | Diff
shouldn't dup() descriptor be also marked like this? (1.33 KB, patch)
2009-02-22 05:24 UTC, Arkadiusz Miskiewicz
Details | Diff
updated pacht which does not remove apr_unix_child_file_cleanup (15.37 KB, patch)
2009-02-23 13:33 UTC, Stefan Fritsch
Details | Diff
updated patch with more error checking and SOCK_CLOEXEC configure test (23.93 KB, patch)
2009-02-24 14:54 UTC, Stefan Fritsch
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Fritsch 2008-12-20 14:59:35 UTC
apr should set FD_CLOEXEC if APR_FOPEN_NOCLEANUP is not set. This would in general help with security and fix some existing issues with third party httpd modules like mod_php:

https://issues.apache.org/bugzilla/show_bug.cgi?id=43965

There was some discussion about this at

http://mail-archives.apache.org/mod_mbox/apr-dev/200808.mbox/<1217901131.25598.58.camel%40shrek.rexursive.com>

Bojan Smojver has created an initial patch:

http://mail-archives.apache.org/mod_mbox/apr-dev/200808.mbox/raw/<1217995221.25598.67.camel%40shrek.rexursive.com>
Comment 1 Stefan Fritsch 2008-12-21 06:04:35 UTC
Created attachment 23045 [details]
Patch for trunk to set FD_CLOEXEC

Here is a patch that sets FD_CLOEXEC when a fd is not inheritable. This has the
following caveats:

- I don't use any of the new APIs (O_CLOEXEC, SOCK_CLOEXEC, ...) because I
  haven't checked what happens if apr is compiled on a system with O_CLOEXEC
  and then is used on a system without O_CLOEXEC.

- For the pollsets, I have only changed epoll so far. Changing the other
  implementations should be straightforward, though.

- I have only dealt with unix (don't know if windows and os2 have something
  like FD_CLOEXEC).

- If this patch is used with apache+mod_php, a program that is called by e.g.
  php's passthru() won't have stderr because the error log is not opened with
  APR_FOPEN_NOCLEANUP.  Maybe apache should set this flag on its error log.
  Or mod_php should take care to provide stderr for its children.

Before I spend more time on this, I would like to know if this approach has
chances of being integrated.
Comment 2 Joe Orton 2009-01-02 03:42:04 UTC
I think it would be good to do this.

Note that any place a particular object is changed to rely on FD_CLOEXEC for closure after exec, the corresponding child cleanup needs to be changed (or if appropriate, removed) since it will otherwise also try to close() the fd for a second time.

Lots of system call invocations:

+        int fdflags = fcntl(fd, F_GETFD);
+        fdflags |= FD_CLOEXEC;
+        fcntl(fd, F_SETFD, fdflags);

without any error checking seems kinda bad, but maybe that's unnecessary paranoia.
Comment 3 Arkadiusz Miskiewicz 2009-02-17 03:49:48 UTC
http://udrepper.livejournal.com/20407.html says "When should these interfaces be used? The answer is simple: whenever the author is not sure that no asynchronous fork()+exec can happen or a concurrently running threads executes fork()+exec (or posix_spawn(), BTW)."

Can these happen when threaded MPM is being used?

ps. dangling descriptors is a pain for me, too and I'm looking forward to see this patch in trunk
Comment 4 Bojan Smojver 2009-02-19 19:05:27 UTC
We really should be using O_CLOEXEC, dup3 and friends were available. As Ulrich explains, there can be a race condition:

"There is a (possibly brief) period of time between the return of the open() call or other function creating a file descriptor and the fcntl() call to set the flag."

Using FD_CLOEXEC is better than nothing, but will miss the above.

> I haven't checked what happens if apr is compiled on a system with O_CLOEXEC and then is used on a system without O_CLOEXEC.

Do we allow this kind of thing to be done at all? After all, we try to detect what is available during configure time, so that we can reliably use that later. All kinds of things will break if APR compiled on one system is used on another that doesn't have those features available.

Take gethostbyname/getservbyname() stuff - we detect that during compile. If we move such APR to another machine that is lacking what we detected, it won't work. Same with O_CLOEXEC, dup3 etc.
Comment 5 Arkadiusz Miskiewicz 2009-02-21 15:33:45 UTC
Created attachment 23288 [details]
updated patch that uses secure way of descriptors handling (see drepper note) where possible

Updated patch. Uses http://udrepper.livejournal.com/20407.html user level when possible and fcntl where not. Also added fcntl() error handling that was missing in original patch.

Passes make test on my Linux platform.

What else is needed to get this patch merged?
Comment 6 Arkadiusz Miskiewicz 2009-02-21 16:10:13 UTC
Created attachment 23289 [details]
updated patch that uses secure way of descriptors handling

Forgot to initialize flags in one place. Fixed.
Comment 7 Stefan Fritsch 2009-02-22 04:33:58 UTC
(In reply to comment #2)
> Note that any place a particular object is changed to rely on FD_CLOEXEC for
> closure after exec, the corresponding child cleanup needs to be changed (or if
> appropriate, removed) since it will otherwise also try to close() the fd for a
> second time.

It is not strictly necessary to change the cleanups because they run before the exec. When a file was closed by the cleanup is no longer relevant if it had FD_CLOEXEC or not. From a performance point of view, the unnecessary cleanups should be removed, of course.

If this patch is ever backported to 1.3/1.4, I would suggest not to remove the cleanups there, to retain maximum compatibility (some apps could mess with the cleanups, after all).

> Lots of system call invocations:
> 
> +        int fdflags = fcntl(fd, F_GETFD);
> +        fdflags |= FD_CLOEXEC;
> +        fcntl(fd, F_SETFD, fdflags);
> 
> without any error checking seems kinda bad, but maybe that's unnecessary
> paranoia.

The only relevant failure modes are EBADF, if the fd is invalid, and EINVAL, if FD_CLOEXEC is unknown. I think we can ignore the latter case and only do error checking on fcntl in cases where we haven't verified that fd is valid, yet.

But I would be fine with adding error checking in all cases, of course.
Comment 8 Stefan Fritsch 2009-02-22 04:41:19 UTC
(In reply to comment #4)
> > I haven't checked what happens if apr is compiled on a system with O_CLOEXEC and then is used on a system without O_CLOEXEC.
> 
> Do we allow this kind of thing to be done at all? After all, we try to detect
> what is available during configure time, so that we can reliably use that
> later. All kinds of things will break if APR compiled on one system is used on
> another that doesn't have those features available.
> 
> Take gethostbyname/getservbyname() stuff - we detect that during compile. If we
> move such APR to another machine that is lacking what we detected, it won't
> work. Same with O_CLOEXEC, dup3 etc.

At least apr should break in an obvious way if one does this. If new syscalls are used, this is ok because the error will be noticed.

But if the flag is silently ignored on older systems as is the case with O_CLOEXEC on linux, this may cause subtle breackage or even create security issues. In this case we should either verify that O_CLOEXEC works during run time (a PITA), or just use fcntl in addition to be on the save side.

If we can be sure that O_CLOEXEC is supported, the additional fcntl call could be ommited. For example, any linux system that supports dup3 also supports O_CLOEXEC.
Comment 9 Stefan Fritsch 2009-02-22 05:06:15 UTC
Created attachment 23293 [details]
updated patch

This updated patch
- is more complete (dev/port, kqueue, mktemp, ...)
- removes unnecessary cleanup functions
- includes Arkadiusz's patch (but some possibilities to use the 
  new api is probably still missing, I don't currently have a
  linux 2.6.28 system).
- compiles with current trunk and passes make test but is otherwise
  untested
- may create problems on netware because it touches files in unix/...
  that are also used by netware (does netware have FD_CLOEXEC?)
Comment 10 Arkadiusz Miskiewicz 2009-02-22 05:24:01 UTC
Created attachment 23294 [details]
shouldn't dup() descriptor be also marked like this?
Comment 11 Stefan Fritsch 2009-02-22 06:48:22 UTC
(In reply to comment #10)
> Created an attachment (id=23294) [details]
> shouldn't dup() descriptor be also marked like this?

I don't think so:

    /* apr_file_dup() retains all old_file flags with the exceptions
     * of APR_INHERIT and APR_FILE_NOCLEANUP.
     * The user must call apr_file_inherit_set() on the dupped
     * apr_file_t when desired.

FD_CLOEXEC should always be unset for the which_dup == 1 case.
Comment 12 Arkadiusz Miskiewicz 2009-02-22 07:41:55 UTC
apr_socket_create in your patch sets CLOEXEC via fcntl even if SOCK_CLOEXEC was used. Looks like missing #ifndef SOCK_CLOEXEC, right?

btw. probably these things need configure checks since for example SOCK_CLOEXEC is supported since 2.6.27 but I already have few users that run older kernels (.25) and have newer glibc that actually defines SOCK_CLOEXEC. The bad thing with this flag is that socket() fails on older kernels and is not ignored.

The most portable way would be to test these things runtime but I don't know if that would fit into "apr way".
Comment 13 Bojan Smojver 2009-02-22 14:54:39 UTC
Pretty sure removing the custom inherit_unset function is a mistake. It is about buffer flushing there - not closing the file. Also, replacing cleanup functions with null functions is an error - same reason.
Comment 14 Bojan Smojver 2009-02-22 15:01:00 UTC
In addition, there is the question of unlinking the file (if required) and removing the mutex. We can't just not have that, I think.
Comment 15 Stefan Fritsch 2009-02-23 12:12:48 UTC
(In reply to comment #13)
> Pretty sure removing the custom inherit_unset function is a mistake. It is
> about buffer flushing there - not closing the file. Also, replacing cleanup
> functions with null functions is an error - same reason.

The problem was that the normal cleanup did flush the buffers. I only removed the child cleanup, because close by FD_CLOEXEC does the right thing in this case.

(In reply to comment #14)
> In addition, there is the question of unlinking the file (if required) and
> removing the mutex. We can't just not have that, I think.

Unlinking the file is not done in the child cleanup (is_child == 0), but you are right about the mutex. I missed that and will post an updated patch. There is also a typo in port.c.

For all other child cleanups than apr_unix_file_child_cleanup, it is ok to remove them because they only close the fd anyway.
Comment 16 Stefan Fritsch 2009-02-23 12:16:15 UTC
(In reply to comment #12)
> apr_socket_create in your patch sets CLOEXEC via fcntl even if SOCK_CLOEXEC was
> used. Looks like missing #ifndef SOCK_CLOEXEC, right?
>
> btw. probably these things need configure checks since for example SOCK_CLOEXEC
> is supported since 2.6.27 but I already have few users that run older kernels
> (.25) and have newer glibc that actually defines SOCK_CLOEXEC. The bad thing
> with this flag is that socket() fails on older kernels and is not ignored.

I assumed that SOCK_CLOEXEC behaved like O_CLOEXEC. But if it is not ignored then there is no reason for the additional fcntl calls. A configure check would be good in this case.
Comment 17 Bojan Smojver 2009-02-23 13:04:38 UTC
> The problem was that the normal cleanup did flush the buffers. I only removed
the child cleanup, because close by FD_CLOEXEC does the right thing in this
case.

True. I misread the patch there.

> Unlinking the file is not done in the child cleanup (is_child == 0)

Not sure about this:
-------------------------
apr_status_t apr_unix_child_file_cleanup(void *thefile)
{
    return file_cleanup(thefile);
}


static apr_status_t file_cleanup(apr_file_t *file)
{
    apr_status_t rv = APR_SUCCESS;

    if (close(file->filedes) == 0) {
        file->filedes = -1;
        if (file->flags & APR_DELONCLOSE) {
            unlink(file->fname);
        }
...
-------------------------

To me, that looks like it does actually unlink the file, even in the child cleanup. No?
Comment 18 Stefan Fritsch 2009-02-23 13:17:17 UTC
(In reply to comment #14)
> > Unlinking the file is not done in the child cleanup (is_child == 0)
> 
> Not sure about this:
> ...
> -------------------------
> 
> To me, that looks like it does actually unlink the file, even in the child
> cleanup. No?

This was changed in trunk in r712674:
-------------------------
apr_status_t apr_unix_child_file_cleanup(void *thefile)
{
    return file_cleanup(thefile, 1);
}

static apr_status_t file_cleanup(apr_file_t *file, int is_child)
{
    apr_status_t rv = APR_SUCCESS;

    if (close(file->filedes) == 0) {
        file->filedes = -1;

        /* Only the parent process should delete the file! */
        if (!is_child && (file->flags & APR_DELONCLOSE)) {
            unlink(file->fname);
        }
...
-------------------------

Maybe that fix should be backported?

Another question: apr_unix_file_child_cleanup destroys the file's thread mutex. But the pollsets also have a thread mutex that is not destroyed by their cleanup function. Is this the correct behaviour?
Comment 19 Bojan Smojver 2009-02-23 13:31:50 UTC
> Maybe that fix should be backported?

Most likely. Not sure why it was never backported, but it does make sense.

> Another question: apr_unix_file_child_cleanup destroys the file's thread mutex.
But the pollsets also have a thread mutex that is not destroyed by their
cleanup function. Is this the correct behaviour?

Not too familiar with that code. Will have to look. Off hand, I would say yes.
Comment 20 Stefan Fritsch 2009-02-23 13:33:11 UTC
Created attachment 23300 [details]
updated pacht which does not remove apr_unix_child_file_cleanup
Comment 21 Arkadiusz Miskiewicz 2009-02-23 13:50:32 UTC
Small suggestion - be safe when fcntl fails:

+        int fdflags = fcntl(fd, F_GETFD);
+        if (fdflags != -1) {
+            fdflags |= FD_CLOEXEC;
+            fcntl(fd, F_SETFD, fdflags);
+        }


ps. I'm currently using fcntl only (not complete, unix only, epoll) patch on few machines with a success (decided to not use new API because it's only suported by very fresh kernels (2.6.23+ for most of things and 2.6.27+ for SOCK_CLOEXEC)).

http://cvs.pld-linux.org/cgi-bin/cvsweb.cgi/SOURCES/apr-bug-46425.patch
Comment 22 Bojan Smojver 2009-02-23 14:30:07 UTC
FYI, I just backported r712674 to 1.3.x.
Comment 23 Bojan Smojver 2009-02-23 15:16:58 UTC
Regarding comment #21, I think what Stefan used in his patch is a better alternative:

+    flags = fcntl((*new)->socketdes, F_GETFD);
+    if (flags == -1)
+        return errno;

If we do what's suggested in comment #21, setting this flag becomes optional. I don't think that's the correct behaviour.

PS. I think the above should be used consistently, which is not the case in the current patch.
Comment 24 Stefan Fritsch 2009-02-24 14:54:16 UTC
Created attachment 23304 [details]
updated patch with more error checking and SOCK_CLOEXEC configure test
Comment 25 Bojan Smojver 2009-02-25 18:48:30 UTC
Regarding comment #24, I had a read of it and it looks pretty good to me.

I committed to trunk. Let's see what the review process reveals. All tests still pass for apr.
Comment 26 Stefan Fritsch 2009-10-11 06:59:27 UTC
This was released with apr 1.3.6