apr_file_close()/apr_socket_close()

[Bojan Smojver]

We have effectively this code closing the file in apr_file_close():
---------------
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;
---------------

If someone calls apr_file_os_get() from another thread (whether they set
APR_FOPEN_XTHREAD or not), then may get a file->filedes which is an FD
of an already closed file. It gets worse - they may get an FD which was
reused by yet another thread. Quite dangerous.

I think it would be safer if we did this:
---------------
static apr_status_t file_cleanup(apr_file_t *file, int is_child)
{
    apr_status_t rv = APR_SUCCESS;
    int fd = file->filedes;

    file->filedes = -1;

    if (close(fd) == 0) {
        [ ... ]
    }
    else {
        [ ... ]
        file->filedes = fd; /* restore, close() was not successful */
    }
---------------

apr_socket_close() is similar.

Opinions?

--
Bojan

[Bojan Smojver]

On Fri, 2009-10-30 at 17:13 +1100, Bojan Smojver wrote:
> It gets worse - they may get an FD which was reused by yet another
> thread. Quite dangerous.

What I am getting at here is a particular situation where the thread
closing of our file has been suspended (say, got a signal and is now in
sigsuspend() within the signal handler, waiting for another signal to be
woken up).

If it so happens that close() just successfully finished and
file->filedes was not set to -1 yet, there is a possibility that the FD
returned by apr_file_os_get() may be an FD reused already by another
thread (or at least, it will be closed).

If we set file->filedes to -1 early, we can at least defend against that
use case.

--
Bojan

[Stefan Ruppert]

Hi Bojan,

first of all the question is if the apr_file_t handle can be safely used
from different threads? e.g. is APR file wrapper thread safe? And I
don't think so! Or am I wrong? If it isn't thread safe the caller is
responsible for locking the involved operations in different threads!

If APR file wrappers should be thread safe, it has to use a mutex to
protect its state changes...

Regards,
Stefan


Bojan Smojver wrote:

> We have effectively this code closing the file in apr_file_close():
> ---------------
> 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;
> ---------------
>
> If someone calls apr_file_os_get() from another thread (whether they set
> APR_FOPEN_XTHREAD or not), then may get a file->filedes which is an FD
> of an already closed file. It gets worse - they may get an FD which was
> reused by yet another thread. Quite dangerous.
>
> I think it would be safer if we did this:
> ---------------
> static apr_status_t file_cleanup(apr_file_t *file, int is_child)
> {
>     apr_status_t rv = APR_SUCCESS;
>     int fd = file->filedes;
>
>     file->filedes = -1;
>
>     if (close(fd) == 0) {
>         [ ... ]
>     }
>     else {
>         [ ... ]
>         file->filedes = fd; /* restore, close() was not successful */
>     }
> ---------------
>
> apr_socket_close() is similar.
>
> Opinions?
>

[Bojan Smojver]

On Fri, 2009-10-30 at 10:26 +0100, Stefan Ruppert wrote:
> first of all the question is if the apr_file_t handle can be safely
> used from different threads? e.g. is APR file wrapper thread safe? And
> I don't think so!

It isn't safe. Not even with APR_FOPEN_XTHREAD.

This whole thing is related to this:

http://www.mail-archive.com/dev@.../msg46051.html
 
--
Bojan

[Stefan Ruppert]

Bojan Smojver wrote:

> On Fri, 2009-10-30 at 10:26 +0100, Stefan Ruppert wrote:
>> first of all the question is if the apr_file_t handle can be safely
>> used from different threads? e.g. is APR file wrapper thread safe? And
>> I don't think so!
>
> It isn't safe. Not even with APR_FOPEN_XTHREAD.
>
> This whole thing is related to this:
>
> http://www.mail-archive.com/dev@.../msg46051.html
>  

I don't know the concept behind APR_FOPEN_XTHREAD, but after a quick
look into the trunk source code this seems to only protect the buffered
io but not the file structure itself.

 From my point of view if an application needs to handle a file within
different threads it has to protect its multi-threaded usage with an own
lock!

However, if you want to be sure use valgrind/helgrind to detect any
threading race conditions. I have found a race condition with helgrind
in apr_pools.c! See my post from august. This tool is incredible!

Stefan

[Bojan Smojver]

On Fri, 2009-10-30 at 12:01 +0100, Stefan Ruppert wrote:
> I don't know the concept behind APR_FOPEN_XTHREAD, but after a quick
> look into the trunk source code this seems to only protect the
> buffered io but not the file structure itself.
>
>  From my point of view if an application needs to handle a file within
> different threads it has to protect its multi-threaded usage with an
> own  lock!

Actually, this problem occurs even if you don't use the same APR file
from the second thread.

Say you have a thread that is just about to close an APR file using
apr_file_close(). It gets interrupted just after call to close(). So,
file->filedes is not -1, but the file is closed.

Then another thread gets scheduled and opens a new file (using just
open(), not APR) and gets an FD that is exactly equal to file->filedes.

Now, the first thread, the one using APR file, in the signal handler
that interrupted it, calls apr_file_os_get() on that file. It gets back
the FD from the second thread! That is a problem that no locking of APR
file can solve. Any action on this FD will do things to the file from
the second thread, not from this one (because it's already closed).

But, by setting file->filedes to -1 before the close(), we can at least
mitigate the consequences of such a situation.

--
Bojan

[Graham Leggett]

Bojan Smojver wrote:

> But, by setting file->filedes to -1 before the close(), we can at least
> mitigate the consequences of such a situation.

+1.

Regards,
Graham
--

[Stefan Ruppert]

Bojan Smojver wrote:
> Now, the first thread, the one using APR file, in the signal handler
> that interrupted it, calls apr_file_os_get() on that file. It gets back
> the FD from the second thread! That is a problem that no locking of APR
> file can solve. Any action on this FD will do things to the file from
> the second thread, not from this one (because it's already closed).
>
> But, by setting file->filedes to -1 before the close(), we can at least
> mitigate the consequences of such a situation.
>

Ok, I got your point. It is not a threading issue. Its a signal handler
issue. For that purpose your proposal works just fine.

Stefan