Bug 46425

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.

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.

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

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.

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?

Created attachment 23289 [details]
updated patch that uses secure way of descriptors handling

Forgot to initialize flags in one place. Fixed.

(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.

(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.

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?)

Created attachment 23294 [details]
shouldn't dup() descriptor be also marked like this?

(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.

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".

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.

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.

(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.

(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.

> 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?

(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?

> 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.

Created attachment 23300 [details]
updated pacht which does not remove apr_unix_child_file_cleanup

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

FYI, I just backported r712674 to 1.3.x.

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.

Created attachment 23304 [details]
updated patch with more error checking and SOCK_CLOEXEC configure test

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.

This was released with apr 1.3.6