Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (nike.apache.org: local policy)
Date: Tue, 18 May 2010 09:18:23 +0200 (CEST)
From: Stefan Fritsch <sf@sfritsch.de>
To: dev@httpd.apache.org
cc: dev@apr.apache.org
Subject: Re: File descriptor leak with mpm-event / apr file bucket cleanup
In-Reply-To: <4BF22B2E.5000304@apache.org>
Message-ID: <alpine.DEB.1.10.1005180916300.30292@eru.sfritsch.de>
References: <201005172311.39558.sf@sfritsch.de> <4BF22B2E.5000304@apache.org>
User-Agent: Alpine 1.10 (DEB 962 2008-03-14)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed

On Tue, 18 May 2010, Ruediger Pluem wrote:
>> --- buckets/apr_buckets_file.c.dist +0200
>> +++ buckets/apr_buckets_file.c
>> @@ -34,8 +34,7 @@
>>      apr_bucket_file *f = data;
>>
>>      if (apr_bucket_shared_destroy(f)) {
>> -        /* no need to close the file here; it will get
>> -         * done automatically when the pool gets cleaned up */
>> +        apr_file_close(f->fd);
>>          apr_bucket_free(f);
>>      }
>>  }
>>
>>
>
> I guess this would be wrong. Think of the following scenario.
>
> 1. A brigade with a file bucket.
> 2. The file bucket is split into two file buckets during processing in the
>   filter chain.
> 3. The first bucket is processed and destroyed. This would leave the second
>   file bucket with an closed file descriptor.
>
> So if you want to close this fd you IMHO would need to do some refcounting
> and only close it if no other filebucket still references it.

The filebuckets already do refcounting. apr_bucket_shared_destroy(f) in 
the patch above is only true if the refcount has reached zero.