Return-Path: Delivered-To: apmail-new-httpd-archive@apache.org Received: (qmail 79429 invoked by uid 500); 6 Jul 2000 23:16:54 -0000 Mailing-List: contact new-httpd-help@apache.org; run by ezmlm Precedence: bulk X-No-Archive: yes Reply-To: new-httpd@apache.org list-help: list-unsubscribe: list-post: Delivered-To: mailing list new-httpd@apache.org Received: (qmail 79417 invoked from network); 6 Jul 2000 23:16:52 -0000 Sender: greg@raleigh.ibm.com Message-ID: <39651320.D9B0CF1C@raleigh.ibm.com> Date: Thu, 06 Jul 2000 19:15:44 -0400 From: Greg Ames X-Mailer: Mozilla 4.7 [en] (X11; I; Linux 2.2.14-15mdk i686) X-Accept-Language: en MIME-Version: 1.0 To: new-httpd@apache.org Subject: Re: [PATCH] mod_file_cache - memory leak References: <3963B2F9.72AB273F@raleigh.ibm.com> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Greg Ames wrote: > > The mmapfile function in mod_file_cache has the same pool usage problem as > the mmapfile function in mod_mmap_static. OK, the small easily reviewable, discrete function patches technique doesn't seem to be working for me. Was there a problem with my previous patch? I'm wasting a lot of time retrofitting the code I'm running with to the current CVS level. This patch fixes the problem mentioned above, and two others: * SIGSEGVs when both MMapFile and CacheFile directives are present. The fix combines the cleanup routines and only cleans up resources when appropriate. * Log sendfile errors. Greg Index: modules/standard/mod_file_cache.c =================================================================== RCS file: /cvs/apache/apache-2.0/src/modules/standard/mod_file_cache.c,v retrieving revision 1.15 diff -u -d -b -r1.15 mod_file_cache.c --- mod_file_cache.c 2000/07/06 15:56:45 1.15 +++ mod_file_cache.c 2000/07/06 22:44:41 @@ -130,7 +130,6 @@ #include "apr_mmap.h" module MODULE_VAR_EXPORT file_cache_module; -static ap_pool_t *pconf; static int once_through = 0; typedef struct { @@ -188,7 +187,6 @@ return rv; } -#if APR_HAS_SENDFILE static ap_status_t cleanup_file_cache(void *sconfv) { a_server_config *sconf = sconfv; @@ -198,30 +196,18 @@ n = sconf->files->nelts; file = (a_file *)sconf->files->elts; while(n) { - ap_close(file->file); - ++file; - --n; - } - return APR_SUCCESS; -} -#endif #if APR_HAS_MMAP -static ap_status_t cleanup_mmap(void *sconfv) -{ - a_server_config *sconf = sconfv; - size_t n; - a_file *file; - - n = sconf->files->nelts; - file = (a_file *)sconf->files->elts; - while(n) { + if (file->is_mmapped) { ap_mmap_delete(file->mm); + } + else +#endif + ap_close(file->file); ++file; --n; } return APR_SUCCESS; } -#endif static const char *cachefile(cmd_parms *cmd, void *dummy, const char *filename) @@ -235,7 +221,7 @@ /* canonicalize the file name? */ /* os_canonical... */ - if (ap_stat(&tmp.finfo, filename, NULL) != APR_SUCCESS) { + if (ap_stat(&tmp.finfo, filename, cmd->temp_pool) != APR_SUCCESS) { ap_log_error(APLOG_MARK, APLOG_WARNING, errno, cmd->server, "file_cache: unable to stat(%s), skipping", filename); return NULL; @@ -285,7 +271,7 @@ a_file tmp; ap_file_t *fd = NULL; - if (ap_stat(&tmp.finfo, filename, pconf) != APR_SUCCESS) { + if (ap_stat(&tmp.finfo, filename, cmd->temp_pool) != APR_SUCCESS) { ap_log_error(APLOG_MARK, APLOG_WARNING, errno, cmd->server, "mod_file_cache: unable to stat(%s), skipping", filename); return NULL; @@ -295,12 +281,13 @@ "mod_file_cache: %s isn't a regular file, skipping", filename); return NULL; } - if (ap_open(&fd, filename, APR_READ, APR_OS_DEFAULT, pconf) != APR_SUCCESS) { + if (ap_open(&fd, filename, APR_READ, APR_OS_DEFAULT, cmd->temp_pool) + != APR_SUCCESS) { ap_log_error(APLOG_MARK, APLOG_WARNING, errno, cmd->server, "mod_file_cache: unable to open(%s, O_RDONLY), skipping", filename); return NULL; } - if (ap_mmap_create(&tmp.mm, fd, 0, tmp.finfo.size, pconf) != APR_SUCCESS) { + if (ap_mmap_create(&tmp.mm, fd, 0, tmp.finfo.size, cmd->pool) != APR_SUCCESS) { int save_errno = errno; ap_close(fd); errno = save_errno; @@ -315,7 +302,7 @@ *new_file = tmp; if (sconf->files->nelts == 1) { /* first one, register the cleanup */ - ap_register_cleanup(cmd->pool, sconf, cleanup_mmap, ap_null_cleanup); + ap_register_cleanup(cmd->pool, sconf, cleanup_file_cache, ap_null_cleanup); } new_file->is_mmapped = TRUE; @@ -344,7 +331,6 @@ int nelts; once_through++; - pconf = p; /* sort the elements of the main_server, by filename */ sconf = ap_get_module_config(s->module_config, &file_cache_module); @@ -421,6 +407,7 @@ struct iovec iov; ap_hdtr_t hdtr; ap_hdtr_t *phdtr = &hdtr; + ap_status_t rv; ap_int32_t flags = 0; /* @@ -451,21 +438,29 @@ flags |= APR_SENDFILE_DISCONNECT_SOCKET; } - iol_sendfile(r->connection->client->iol, + rv = iol_sendfile(r->connection->client->iol, file->file, phdtr, &offset, &length, flags); + if (rv != APR_SUCCESS) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, + "mod_file_cache: iol_sendfile failed."); } + } else { while (ap_each_byterange(r, &offset, &length)) { - iol_sendfile(r->connection->client->iol, + rv =iol_sendfile(r->connection->client->iol, file->file, phdtr, &offset, &length, - flags); + 0); + if (rv != APR_SUCCESS) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, + "mod_file_cache: iol_sendfile failed."); + } phdtr = NULL; } }