Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: neutral (nike.apache.org: local policy)
Message-ID: <4AEAB13F.3000804@ruppert-it.de>
Date: Fri, 30 Oct 2009 10:26:23 +0100
From: Stefan Ruppert <ml@ruppert-it.de>
User-Agent: Mozilla-Thunderbird 2.0.0.22 (X11/20090707)
MIME-Version: 1.0
To: APR Development List <dev@apr.apache.org>
Subject: Re: apr_file_close()/apr_socket_close()
References: <1256883239.2488.54.camel@shrek.rexursive.com>
In-Reply-To: <1256883239.2488.54.camel@shrek.rexursive.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

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