You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Greg Ames <gr...@raleigh.ibm.com> on 2000/07/06 00:13:13 UTC
[PATCH] mod_file_cache - memory leak
The mmapfile function in mod_file_cache has the same pool usage problem as the mmapfile
function in mod_mmap_static. Also take care of a warning under Win32.
Greg
Index: mod_file_cache.c
===================================================================
RCS file: /cvs/apache/apache-2.0/src/modules/standard/mod_file_cache.c,v
retrieving revision 1.13
diff -u -d -b -r1.13 mod_file_cache.c
--- mod_file_cache.c 2000/07/05 18:52:57 1.13
+++ mod_file_cache.c 2000/07/05 21:46:21
@@ -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 {
@@ -167,7 +166,7 @@
* support.
*/
HANDLE hFile;
- hFile = CreateFile(filename, /* pointer to name of the file */
+ hFile = CreateFile((char *) filename, /* pointer to name of the file */
GENERIC_READ, /* access (read-write) mode */
FILE_SHARE_READ, /* share mode */
NULL, /* pointer to security attributes */
@@ -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;
Re: [PATCH] mod_file_cache - memory leak
Posted by Greg Ames <gr...@raleigh.ibm.com>.
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;
}
}
Re: [PATCH] mod_file_cache - memory leak
Posted by Greg Ames <gr...@raleigh.ibm.com>.
rbb@covalent.net wrote:
> On Wed, 5 Jul 2000, Greg Ames wrote:
>
> > The mmapfile function in mod_file_cache has the same pool usage problem as the mmapfile
> > function in mod_mmap_static. Also take care of a warning under Win32.
>
> I thought these were going to be combined at some point. They serve
> basically the same function.
>
> Ryan
Yeah, they are being combined, but mod_file_cache isn't quite ready for prime time on Unix.
I've got another patch for SEGVs ready to go after this patch is committed. I'm trying to
be a good doobie and separate my patches into easily reviewable, discrete units :-) but that
really slows down the process when this mod need a bunch of fixes.
btw, the cast for Win32 is not needed - CreateFile takes a const char * filename.
Greg
Re: [PATCH] mod_file_cache - memory leak
Posted by rb...@covalent.net.
On Wed, 5 Jul 2000, Greg Ames wrote:
> The mmapfile function in mod_file_cache has the same pool usage problem as the mmapfile
> function in mod_mmap_static. Also take care of a warning under Win32.
I thought these were going to be combined at some point. They serve
basically the same function.
Ryan