Crash in apr_file_close() (Linux)

[Yuri V. Vishnevskiy]

Dear developers,
For some reason in my program I need two file handlers of stdout stream.
When I close these handlers I obtain an error on the second close: "Bad  
file descriptor"

Here is the simple testcase code:


#include <stdlib.h>
#include <stdio.h>
#include "apr.h"
#include "apr_pools.h"
#include "apr_file_io.h"

int main(int a, char** b)
{
apr_pool_t * pool;
apr_file_t *file1, *file2;
apr_status_t status;
char errbuf[100];


  apr_initialize();
  apr_pool_create(&pool, NULL);

  apr_file_open_stdout(&file1, pool);
  apr_file_open_stdout(&file2, pool);

  apr_file_puts("Hello World (1)\n", file1);
  apr_file_puts("Hello World (2)\n", file2);

  status = apr_file_close(file1);
  if(status!=APR_SUCCESS) {
    apr_strerror(status, errbuf, sizeof(errbuf));
    fprintf(stderr, "file1 close error:  %s\n", errbuf);
  }

  status = apr_file_close(file2); // <--- returns non-APR_SUCCESS
  if(status!=APR_SUCCESS) {
    apr_strerror(status, errbuf, sizeof(errbuf));
    fprintf(stderr, "file2 close error:  %s\n", errbuf);
  }

  apr_terminate();

  return(0);
}


Could anybody try to reproduce this problem?
If it really exists, should it be fixed in APR or it is my problem to  
handle such things?

I use APR 1.3.8
My system is Linux Debian Squeeze, x86_64.

Regards,
Yura Vishnevskiy

[Bojan Smojver]

On Fri, 2009-10-16 at 15:26 +0700, Yuri V. Vishnevskiy wrote:
> For some reason in my program I need two file handlers of stdout
> stream.

If you duplicate the descriptor with apr_file_dup() instead of calling
apr_file_open_stdout() twice, does it still do the same?

--
Bojan

[Joe Orton]

On Fri, Oct 16, 2009 at 03:26:31PM +0700, Yuri V. Vishnevskiy wrote:
> Dear developers,
> For some reason in my program I need two file handlers of stdout stream.
> When I close these handlers I obtain an error on the second close: "Bad  
> file descriptor"

It seems like reasonable behaviour that the second apr_file_close() call
should fail, though it certainly isn't obvious from reading the docs.  
How else do you expect it to behave?  You cannot close an fd twice.

If you want to be able to close the fds without closing stdout, then use
apr_file_dup() to create the second handle to stdout.

Regards, Joe

[mturk]

On 16/10/09 10:26, Yuri V. Vishnevskiy wrote:
> Dear developers,
> For some reason in my program I need two file handlers of stdout stream.
>
> Could anybody try to reproduce this problem?

Crash happens on windows as well, so it might be a real
thing.
If you need two file objects perhaps a dup could help.
Anyhow crash should not happen or at least we should
not that opening stdsream twice could make a crash.


Regards
--
^TM

[Yuri V. Vishnevskiy]

> On Fri, 2009-10-16 at 15:26 +0700, Yuri V. Vishnevskiy wrote:
>> For some reason in my program I need two file handlers of stdout
>> stream.
>
> If you duplicate the descriptor with apr_file_dup() instead of calling
> apr_file_open_stdout() twice, does it still do the same?
>


With apr_file_dup() everything is fine, no errors.

[mturk]

On 16/10/09 10:39, Joe Orton wrote:
>
> It seems like reasonable behaviour that the second apr_file_close() call
> should fail, though it certainly isn't obvious from reading the docs.
> How else do you expect it to behave?  You cannot close an fd twice.
>

I suppose it shouldn't crash the apr.
IMO either second call to open should fail or
the call to close should fail, but without actually calling the close().

Regards
--
^TM

[Joe Orton]

On Fri, Oct 16, 2009 at 10:54:13AM +0200, Mladen Turk wrote:

> On 16/10/09 10:39, Joe Orton wrote:
>>
>> It seems like reasonable behaviour that the second apr_file_close() call
>> should fail, though it certainly isn't obvious from reading the docs.
>> How else do you expect it to behave?  You cannot close an fd twice.
>>
>
> I suppose it shouldn't crash the apr.
> IMO either second call to open should fail or
> the call to close should fail, but without actually calling the close().

I think the reporter meant "fail" when they said "crash".

I don't see any reason why it would crash on Unix: the first call to
apr_file_close() will set fd->filedes to -1 after calling close(), the
second one will call close(-1) and fail with EBADF.

Regards, Joe

[mturk]

On 16/10/09 11:01, Joe Orton wrote:
>
> I think the reporter meant "fail" when they said "crash".
>

Dunno what he meant but I confirmed last week's almost exact
report that crashes (really) windows.

> I don't see any reason why it would crash on Unix: the first call to
> apr_file_close() will set fd->filedes to -1 after calling close(), the
> second one will call close(-1) and fail with EBADF.
>

each invocation to apr_file_open_stdout creates a new apr_file_t so
each fd->filedes has a value of STDOUT_FILENO.
This is the same as calling:
close(1);
close(1);

This cores on Windows with latest MSVCRT, so it might crash
some Unix versions as well?

Regards
--
^TM

[Yuri V. Vishnevskiy]

> On Fri, Oct 16, 2009 at 10:54:13AM +0200, Mladen Turk wrote:
>> On 16/10/09 10:39, Joe Orton wrote:
>>>
>>> It seems like reasonable behaviour that the second apr_file_close()  
>>> call
>>> should fail, though it certainly isn't obvious from reading the docs.
>>> How else do you expect it to behave?  You cannot close an fd twice.
>>>
>>
>> I suppose it shouldn't crash the apr.
>> IMO either second call to open should fail or
>> the call to close should fail, but without actually calling the close().
>
> I think the reporter meant "fail" when they said "crash".
>
> I don't see any reason why it would crash on Unix: the first call to
> apr_file_close() will set fd->filedes to -1 after calling close(), the
> second one will call close(-1) and fail with EBADF.

I have two separate file descriptors. Second apr_file_close(file2) will  
fail even if file2->filedes==1.

So I understand that I have to handle such things by myself.

But if I can't close second handle why it is possible to open it  
successfully?

Regards,
Yura

[Joe Orton]

On Fri, Oct 16, 2009 at 11:17:17AM +0200, Mladen Turk wrote:
> each invocation to apr_file_open_stdout creates a new apr_file_t so
> each fd->filedes has a value of STDOUT_FILENO.
> This is the same as calling:
> close(1);
> close(1);

Oh, good point.  This is rather horrible :(.  I'm not sure what can be
done about it, we can't change apr_file_open_stdout() to do a dup()
internally, since that would break existing apps.

Regards, Joe

[Yuri V. Vishnevskiy]

On Fri, 16 Oct 2009 15:39:49 +0700, Joe Orton <jorton@...> wrote:

> On Fri, Oct 16, 2009 at 03:26:31PM +0700, Yuri V. Vishnevskiy wrote:
>> Dear developers,
>> For some reason in my program I need two file handlers of stdout stream.
>> When I close these handlers I obtain an error on the second close: "Bad
>> file descriptor"
>
> It seems like reasonable behaviour that the second apr_file_close() call
> should fail, though it certainly isn't obvious from reading the docs.
> How else do you expect it to behave?  You cannot close an fd twice.

I understand that very clearly. What I do not understand well is the APR  
file API policy, since apr_file_ functions are general for both usual  
files and std*.

[mturk]

On 16/10/09 11:21, Joe Orton wrote:

> On Fri, Oct 16, 2009 at 11:17:17AM +0200, Mladen Turk wrote:
>> each invocation to apr_file_open_stdout creates a new apr_file_t so
>> each fd->filedes has a value of STDOUT_FILENO.
>> This is the same as calling:
>> close(1);
>> close(1);
>
> Oh, good point.  This is rather horrible :(.  I'm not sure what can be
> done about it, we can't change apr_file_open_stdout() to do a dup()
> internally, since that would break existing apps.
>

I'm not sure why do we call close(0..2) for std files when
the apr_file_open_std* is closed or it's pool destroyed at the
first place.

Reading the:
  * @remark The only reason that the apr_file_open_std* functions exist
  * is that you may not always have a stderr/out/in on Windows.  This
  * is generally a problem with newer versions of Windows and services.

So apr_file_open_std* should just reference the existing
system descriptors. However it behaves like 'attach' rather then open,
meaning once you open it you will destroy the system stdin/stdout/stderr.
Thus by design it's a singleton operation, so if desired,
the second invocation during the process lifetime should always
return EBADF for apr_file_open_std* thought.


Very confusing.


Regards
--
^TM

[Yuri V. Vishnevskiy]

> I'm not sure why do we call close(0..2) for std files when
> the apr_file_open_std* is closed or it's pool destroyed at the
> first place.
>
> Reading the:
>   * @remark The only reason that the apr_file_open_std* functions exist
>   * is that you may not always have a stderr/out/in on Windows.  This
>   * is generally a problem with newer versions of Windows and services.
>
> So apr_file_open_std* should just reference the existing
> system descriptors. However it behaves like 'attach' rather then open,
> meaning once you open it you will destroy the system stdin/stdout/stderr.
> Thus by design it's a singleton operation, so if desired,
> the second invocation during the process lifetime should always
> return EBADF for apr_file_open_std* thought.
>
>
> Very confusing.

This is related with another my problem. If I close stdout by  
apr_file_close, then the standard printf and fprintf(stdout, ) functions  
are no more working.
I've tested this on Windows and Linux.

Regards,
Yura

[Jonathan Leffler-2]

On Fri, Oct 16, 2009 at 7:31 AM, Yuri V. Vishnevskiy <yuri.vishnevskiy@...> wrote:

This is related with another my problem. If I close stdout by apr_file_close, then the standard printf and fprintf(stdout, ) functions are no more working.
I've tested this on Windows and Linux.

If you close standard output, functions that write to standard output should not succeed - there is nowhere for the information to go.  That is (or, IMO, should be) expected behaviour.

--
Jonathan Leffler <jonathan.leffler@...>  #include <disclaimer.h>
Guardian of DBD::Informix - v2008.0513 - http://dbi.perl.org
"Blessed are we who can laugh at ourselves, for we shall never cease to be amused."

[mturk]

On 16/10/09 16:41, Jonathan Leffler wrote:

>
> On Fri, Oct 16, 2009 at 7:31 AM, Yuri V. Vishnevskiy
> <yuri.vishnevskiy@... <mailto:yuri.vishnevskiy@...>> wrote:
>
>     This is related with another my problem. If I close stdout by
>     apr_file_close, then the standard printf and fprintf(stdout, )
>     functions are no more working.
>     I've tested this on Windows and Linux.
>
>
> If you close standard output, functions that write to standard output
> should not succeed - there is nowhere for the information to go.  That
> is (or, IMO, should be) expected behaviour.
>

What 99% users would need is apr_file_attach_std* set of functions
giving the apr_file_t capable API without destroying the system
std streams.

And we should probably protect the sigleton apr_file_open_std*
against multiple invocations, cause the always acts upon a
singleton std stream object.


Regards
--
^TM

[William A. Rowe Jr.]

Mladen Turk wrote:
>
> What 99% users would need is apr_file_attach_std* set of functions
> giving the apr_file_t capable API without destroying the system
> std streams.
>
> And we should probably protect the sigleton apr_file_open_std*
> against multiple invocations, cause the always acts upon a
> singleton std stream object.

We already have it.

Simply don't call close() :)

Just check if apr is registering closure of these streams and perhaps ensure
they are not closed at all if they are not overridden, certainly by apr 2.0.

[Yuri V. Vishnevskiy]

>
> If you close standard output, functions that write to standard output  
> should
> not succeed - there is nowhere for the information to go.  That is (or,  
> IMO,
> should be) expected behaviour.

No doubt. Then how should I to use std* after apr_terminate()?

[William A. Rowe Jr.]

Yuri V. Vishnevskiy wrote:
>>
>> If you close standard output, functions that write to standard output
>> should
>> not succeed - there is nowhere for the information to go.  That is
>> (or, IMO,
>> should be) expected behaviour.
>
> No doubt. Then how should I to use std* after apr_terminate()?

Once you call apr_terminate(), all bets are off if you use such posix entities
of apr objects.  If apr_terminate() is closing the system-provided stdin/out/err,
that is something we can fix either in 1.x (if it is inconsistent/ill defined)
or 2.0 for certain.

[mturk]

On 16/10/09 16:56, William A. Rowe, Jr. wrote:

> Mladen Turk wrote:
>>
>> What 99% users would need is apr_file_attach_std* set of functions
>> giving the apr_file_t capable API without destroying the system
>> std streams.
>>
>> And we should probably protect the sigleton apr_file_open_std*
>> against multiple invocations, cause the always acts upon a
>> singleton std stream object.
>
> We already have it.
>
> Simply don't call close() :)
>

Sure. However we should at least return EBADF on a second
invocation cause we know the first one will destroy stdout when
closed (directly, via pool destroy or via terminate)

For 2.0 apr_file_attach_std* might be a valuable addition
for repetitive apr_file_t objects allowing file API without
jeopardizing the stream integrity.


Regards
--
^TM

[William A. Rowe Jr.]

Mladen Turk wrote:

> On 16/10/09 16:56, William A. Rowe, Jr. wrote:
>> Mladen Turk wrote:
>>>
>>> What 99% users would need is apr_file_attach_std* set of functions
>>> giving the apr_file_t capable API without destroying the system
>>> std streams.
>>>
>>> And we should probably protect the sigleton apr_file_open_std*
>>> against multiple invocations, cause the always acts upon a
>>> singleton std stream object.
>>
>> We already have it.
>>
>> Simply don't call close() :)
>>
>
> Sure. However we should at least return EBADF on a second
> invocation cause we know the first one will destroy stdout when
> closed (directly, via pool destroy or via terminate)

apr_file_open_std* should not auto-close the underlying file stream ever,
only the apr_file_t pointer.

If apr_file_t is then dup'ed to a std* stream, then again all bets will be
off once apr_terminate is invoked.  Perhaps all will be well, but no assurance.

> For 2.0 apr_file_attach_std* might be a valuable addition
> for repetitive apr_file_t objects allowing file API without
> jeopardizing the stream integrity.

In 2.0, we can simply change such semantics.