Re: svn propchange: r747990 - svn:log

[Bojan Smojver <bo...@rexursive.com>]

On Sat, 2009-02-28 at 17:27 +0100, Stefan Fritsch wrote:

> No problem. Thanks for commiting the patch. BTW, I think a CHANGES 
> entry would be good, too. And Arkadiusz Miskiewicz did a part of the 
> patch.

Done.

> Are parts of this suitable for backport to 1.3/1.4?

I would hope so.

> The change for 
> files is probably not, as it can leave exec'ed programs without 
> stdin/stdout/stderr.

We just need to make sure these fds are marked so that they can be
inherited, right?

-- 
Bojan

Re: svn propchange: r747990 - svn:log

[Bojan Smojver <bo...@rexursive.com>]

On Tue, 2009-03-03 at 22:57 +0100, Stefan Fritsch wrote:

> Yes (with "old_file->flags" instead of "flag").

Cut'n'paste is a wonderful thing. Most of the time ;-)

-- 
Bojan

Re: svn propchange: r747990 - svn:log

[Stefan Fritsch <sf...@sfritsch.de>]

On Tuesday 03 March 2009, Bojan Smojver wrote:
> On Tue, 2009-03-03 at 21:51 +0100, Stefan Fritsch wrote:
> > The problem with the CLOEXEC-patch is that apr_file_dup2() now
> > only checks for APR_INHERIT but not for APR_FILE_NOCLEANUP to
> > determine if FD_CLOEXEC should be set. But APR_FILE_NOCLEANUP
> > should imply APR_INHERIT and the new apr_file_dup2 needs to be
> > changed.
>
> Did you mean like this?

Yes (with "old_file->flags" instead of "flag").

Re: svn propchange: r747990 - svn:log

[Bojan Smojver <bo...@rexursive.com>]

On Tue, 2009-03-03 at 21:51 +0100, Stefan Fritsch wrote:

> The problem with the CLOEXEC-patch is that apr_file_dup2() now only 
> checks for APR_INHERIT but not for APR_FILE_NOCLEANUP to determine if 
> FD_CLOEXEC should be set. But APR_FILE_NOCLEANUP should imply 
> APR_INHERIT and the new apr_file_dup2 needs to be changed.

This makes sense to me. In fact, INHERIT macros explicitly depend on
NOCLEANUP flag being unset in order to take action. So, the two
obviously must be related.

-- 
Bojan

Re: svn propchange: r747990 - svn:log

[Bojan Smojver <bo...@rexursive.com>]

On Tue, 2009-03-03 at 18:08 -0600, William A. Rowe, Jr. wrote:

> I believe that is correct.  Anyone else care to validate?  As far as I know
> this change is a noop, until the introduction of this most recent file
> inheritance patch for CLOEXEC.

After reading apr_os_file_put() (which is used to open
stdin/stdout/stderr), it seems that we have this in there:

(*file)->flags = flags | APR_FILE_NOCLEANUP;

Meaning, no cleanup will be registered for this file. If this flag is
set, inheritance setting won't work either (we explicitly go out of
those macros with APR_EINVAL if APR_FILE_NOCLEANUP is set), so nobody
can set such files to be CLOEXEC through APR by mistake. In other words,
they should always be inherited, unless CLOEXEC was used on the OS level
to open stdin/stdout/stderr.

Does that make sense?

-- 
Bojan

Re: svn propchange: r747990 - svn:log

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Bojan Smojver wrote:
> On Tue, 2009-03-03 at 17:57 -0600, William A. Rowe, Jr. wrote:
> 
>> Setting the flag is sufficient.
>>
>> The fact that a handle shouldn't be closed on exec in an apr-sense makes
>> no difference to whether it should be closed on non-apr exec() calls.
> 
> Just before I make the change, let me understand this correctly. What
> you're saying is that we should not then call apr_file_inherit_set()
> here, because it will change that logic even on non-apr exec(), but
> rather simply do inside apr_file_open_stdin/stdout/stderr():
> 
> thefile->flags |= APR_INHERIT;
> 
> Correct?

I believe that is correct.  Anyone else care to validate?  As far as I know
this change is a noop, until the introduction of this most recent file
inheritance patch for CLOEXEC.

Re: svn propchange: r747990 - svn:log

[Bojan Smojver <bo...@rexursive.com>]

On Tue, 2009-03-03 at 17:57 -0600, William A. Rowe, Jr. wrote:

> Setting the flag is sufficient.
> 
> The fact that a handle shouldn't be closed on exec in an apr-sense makes
> no difference to whether it should be closed on non-apr exec() calls.

Just before I make the change, let me understand this correctly. What
you're saying is that we should not then call apr_file_inherit_set()
here, because it will change that logic even on non-apr exec(), but
rather simply do inside apr_file_open_stdin/stdout/stderr():

thefile->flags |= APR_INHERIT;

Correct?

-- 
Bojan

Re: svn propchange: r747990 - svn:log

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Bojan Smojver wrote:
> On Tue, 2009-03-03 at 17:02 -0600, William A. Rowe, Jr. wrote:
> 
>> No; the apr_file_open_stderr structure is still -wrong-.
> 
> Maybe we should then just call apr_file_inherit_set() inside
> apr_file_open_stdin/stdout/stderr() to make sure these get inherited. Is
> this what you were referring to?

Setting the flag is sufficient.

The fact that a handle shouldn't be closed on exec in an apr-sense makes
no difference to whether it should be closed on non-apr exec() calls.

Re: svn propchange: r747990 - svn:log

[Bojan Smojver <bo...@rexursive.com>]

On Tue, 2009-03-03 at 17:02 -0600, William A. Rowe, Jr. wrote:

> No; the apr_file_open_stderr structure is still -wrong-.

Maybe we should then just call apr_file_inherit_set() inside
apr_file_open_stdin/stdout/stderr() to make sure these get inherited. Is
this what you were referring to?

-- 
Bojan

Re: svn propchange: r747990 - svn:log

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Bojan Smojver wrote:
> On Tue, 2009-03-03 at 21:51 +0100, Stefan Fritsch wrote:
> 
>> The problem with the CLOEXEC-patch is that apr_file_dup2() now only 
>> checks for APR_INHERIT but not for APR_FILE_NOCLEANUP to determine if 
>> FD_CLOEXEC should be set. But APR_FILE_NOCLEANUP should imply 
>> APR_INHERIT and the new apr_file_dup2 needs to be changed.
> 
> Did you mean like this?

No; the apr_file_open_stderr structure is still -wrong-.

Bill

Re: svn propchange: r747990 - svn:log

[Bojan Smojver <bo...@rexursive.com>]

On Tue, 2009-03-03 at 21:51 +0100, Stefan Fritsch wrote:

> The problem with the CLOEXEC-patch is that apr_file_dup2() now only 
> checks for APR_INHERIT but not for APR_FILE_NOCLEANUP to determine if 
> FD_CLOEXEC should be set. But APR_FILE_NOCLEANUP should imply 
> APR_INHERIT and the new apr_file_dup2 needs to be changed.

Did you mean like this?

-- 
Bojan

Re: svn propchange: r747990 - svn:log

[Stefan Fritsch <sf...@sfritsch.de>]

On Tuesday 03 March 2009, William A. Rowe, Jr. wrote:
> Let's review what -should- happen today;
>
>      apr_file_open_stderr(&stderr_log, p)
>
> should return stderr_log apr_file_t indicating the INHERIT flag
> set.

OK, this was the missing bit. apr_file_open_stderr() uses 
apr_os_file_put(), which sets APR_FILE_NOCLEANUP but not APR_INHERIT.

> Then; 
>
>      apr_file_dup2(stderr_log, new_file, p)
>
> should maintain the INHERIT flag on the newly dup2'ed stderr_log.
>
> Does that make sense?

The problem with the CLOEXEC-patch is that apr_file_dup2() now only 
checks for APR_INHERIT but not for APR_FILE_NOCLEANUP to determine if 
FD_CLOEXEC should be set. But APR_FILE_NOCLEANUP should imply 
APR_INHERIT and the new apr_file_dup2 needs to be changed.

When I have some free cycles, I will check if this solves the problem 
that processes exec'ed by httpd/mod_php have no stderr.

Cheers,
Stefan

Re: svn propchange: r747990 - svn:log

[Bojan Smojver <bo...@rexursive.com>]

On Mon, 2009-03-02 at 17:07 -0600, William A. Rowe, Jr. wrote:

> This entire patch set has me increasingly worried, as it has the strong
> potential of destabilizing many apr apps.

The _intention_ of the patch (at least as interpreted by me) is that if
the file has APR_INHERIT_SET flag turned _off_, then we also flag the fd
with CLOEXEC. Otherwise, we don't.

So, if someone opened a file through APR file API with an inherit flag
turned off, we now guarantee that this fd will be closed even when the
fork/exec was _not_ done through APR.

Whether we actually implemented that correctly in the patch is another
matter. More eyes welcome here.

Now, if people were relying/exploiting these fds being opened across
fork/exec calls that are not done via APR, then we have a security issue
in the current code. This is what O_CLOEXEC is all about - no free fds
for anyone, unless explicitly given. No?

> Let's review what -should- happen today;
> 
>      apr_file_open_stderr(&stderr_log, p)
> 
> should return stderr_log apr_file_t indicating the INHERIT flag set.  Then;
> 
>      apr_file_dup2(stderr_log, new_file, p)
> 
> should maintain the INHERIT flag on the newly dup2'ed stderr_log.
> 
> Does that make sense?

It does.

-- 
Bojan

Re: svn propchange: r747990 - svn:log

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Stefan Fritsch wrote:
> 
> Do you think it would be ok if this changed for 1.3/1.4 for 
> stdin/out/err?

This entire patch set has me increasingly worried, as it has the strong
potential of destabilizing many apr apps.  I'm already -1 on this for the
1.3 release, and am torn between -0 and -1 to port this to 1.4.  (I have
no issues with new behaviors in 2.0, there is no contract).  Who knows
how many applications are relying on the "broken" (your definition)
behavior of APR, and please remember this is a shared, often system lib.

Let's review what -should- happen today;

     apr_file_open_stderr(&stderr_log, p)

should return stderr_log apr_file_t indicating the INHERIT flag set.  Then;

     apr_file_dup2(stderr_log, new_file, p)

should maintain the INHERIT flag on the newly dup2'ed stderr_log.

Does that make sense?

Re: svn propchange: r747990 - svn:log

[Stefan Fritsch <sf...@sfritsch.de>]

On Monday 02 March 2009, William A. Rowe, Jr. wrote:
> Stefan Fritsch wrote:
> > Who needs to make sure, the application or apr? Currenty httpd
> > 2.2.x does not mark stderr as inheritable. It doesn't sound good
> > if a change in stable apr requires a change in stable httpd. But
> > I guess you could argue that apps relying on the current
> > behaviour are buggy. Or do you want to add special case code into
> > apr for
> > stdin/stdout/stderr?
>
> There is a special case in apr_file_dup for stdin/out/err.

There is some special case logic in file_io/win32/filedup.c but AFAICS 
it does not concern APR_INHERIT/APR_FILE_NOCLEANUP.

file_io/unix/filedup.c has:

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

Do you think it would be ok if this changed for 1.3/1.4 for 
stdin/out/err?

Re: svn propchange: r747990 - svn:log

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Stefan Fritsch wrote:
> 
> Who needs to make sure, the application or apr? Currenty httpd 2.2.x 
> does not mark stderr as inheritable. It doesn't sound good if a 
> change in stable apr requires a change in stable httpd. But I guess 
> you could argue that apps relying on the current behaviour are buggy.
> Or do you want to add special case code into apr for 
> stdin/stdout/stderr?

There is a special case in apr_file_dup for stdin/out/err.

Re: svn propchange: r747990 - svn:log

[Stefan Fritsch <sf...@sfritsch.de>]

On Sunday 01 March 2009, Bojan Smojver wrote:
> > The change for
> > files is probably not, as it can leave exec'ed programs without
> > stdin/stdout/stderr.
>
> We just need to make sure these fds are marked so that they can be
> inherited, right?

Who needs to make sure, the application or apr? Currenty httpd 2.2.x 
does not mark stderr as inheritable. It doesn't sound good if a 
change in stable apr requires a change in stable httpd. But I guess 
you could argue that apps relying on the current behaviour are buggy.
Or do you want to add special case code into apr for 
stdin/stdout/stderr?