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...@remulak.net> on 2001/08/14 21:50:55 UTC
[PATCH] for mod_mime seg fault in 2.0.23 :-)
Bill Stoddard wrote:
>
> The problem is that we are creating an extension_info out of a subrequest pool and
> sticking it into a longer lived hash table (allocated out of the request pool I'm
> guessing). When we pull the extension_info out of the hash table later and try to use it,
> we seg fault because the storage has been reused for something else. No thoughts on a fix.
This fixes the seg fault on my Linux box:
Index: modules/http/mod_mime.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/mod_mime.c,v
retrieving revision 1.50
diff -u -d -b -r1.50 mod_mime.c
--- modules/http/mod_mime.c 2001/08/14 02:35:55 1.50
+++ modules/http/mod_mime.c 2001/08/14 19:39:45
@@ -220,13 +220,10 @@
attrib_info *suffix;
if (base->extension_mappings && add->extension_mappings) {
- if (base->copy_mappings)
- new->extension_mappings = base->extension_mappings;
- else {
new->extension_mappings = apr_hash_make(p);
overlay_extension_mappings(p, base->extension_mappings,
new->extension_mappings);
- }
+
overlay_extension_mappings(p, add->extension_mappings,
new->extension_mappings);
We can't allow overlay_extension_mappings to store things into the
extension_mappings hash table which have a shorter lifetime than the
hash table itself. Always using a new extension_mappings prevents this
from happening.
I'll commit this in a little while unless someone objects soon.
Greg
Greg
Re: [PATCH] I prefer this... Re: [PATCH] for mod_mime seg fault in
2.0.23 :-)
Posted by Greg Ames <gr...@remulak.net>.
Bill Stoddard wrote:
>
> I prefer this patch which creates a completely new, fully merged, mime_dir_config on each
> call to merge_mime_dir_configs. It is a bit more expensive but should it still work better
> than the table code (maybe. I'd like to see it profiled :-). And this code is a hell of a
> lot easier to understand :-)
OK, I like simplicity too. But unfortunately I got:
patching file modules/http/mod_mime.c
Hunk #4 FAILED at 212.
Hunk #5 FAILED at 231.
Hunk #6 FAILED at 245.
Hunk #7 FAILED at 259.
Hunk #9 succeeded at 769 with fuzz 1.
4 out of 9 hunks FAILED -- saving rejects to file
modules/http/mod_mime.c.rej
...when I tried it. Was this patch against the current HEAD?
Greg
Re: [PATCH] I prefer this... Re: [PATCH] for mod_mime seg fault in 2.0.23 :-)
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
The base config is created in cfg->pool (a child of process->pool.)
If we are supposed to merge into the r->pool, we need a copy. That copy gets
tagged with the r->pool
If we are then supposed to merge using the sub_req->pool, we need another copy.
That copy is tagged with that sub_req->pool.
If we start caching configs, each cached <Directory /> config needs it's own pool
(so it can be destroyed when the cache stales.) That cache->pool is different
than the base cfg->pool, so we must copy. When we take the cached config into the
request, cache->pool isn't r->pool, so again we copy.
Other than that, we can keep merging without copies. Otherwise we drop the hashing,
because it is a cpu buster.
Bill
----- Original Message -----
From: "Bill Stoddard" <bi...@wstoddard.com>
To: <ne...@apache.org>
Sent: Tuesday, August 14, 2001 4:21 PM
Subject: Re: [PATCH] I prefer this... Re: [PATCH] for mod_mime seg fault in 2.0.23 :-)
>
> > -1 (veto) to both suggestions.
> >
> > This is getting obscene. We've lost all value of tables, since they can't be memcpy'ed,
> > they have to be reconstructed. This will be much slower than the original array code.
> >
> > Committers, please review dir_create() and dir_merge() patches more carefully. There
> > is a long tradition of configuration bugs due to these assumptions. The patches that
> > added copy/copy_mappings was an attempt to salvage this.
> >
> > I'll demonstrate a solution, using pool scopes, that adds safety and speed. Give me a
> day.
>
> What are pool scopes?
>
> Bill
>
>
Re: [PATCH] I prefer this... Re: [PATCH] for mod_mime seg fault in 2.0.23 :-)
Posted by Bill Stoddard <bi...@wstoddard.com>.
> -1 (veto) to both suggestions.
>
> This is getting obscene. We've lost all value of tables, since they can't be memcpy'ed,
> they have to be reconstructed. This will be much slower than the original array code.
>
> Committers, please review dir_create() and dir_merge() patches more carefully. There
> is a long tradition of configuration bugs due to these assumptions. The patches that
> added copy/copy_mappings was an attempt to salvage this.
>
> I'll demonstrate a solution, using pool scopes, that adds safety and speed. Give me a
day.
What are pool scopes?
Bill
Re: [PATCH] solves the mod_mime seg fault without loosing the hash?
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Brian Pane" <bp...@pacbell.net>
Sent: Wednesday, August 15, 2001 12:05 AM
> William A. Rowe, Jr. wrote:
> [...]
>
> >+static void remove_items(apr_pool_t *p, apr_array_header_t *remove,
> >+ apr_hash_t *mappings, apr_size_t member)
> >+{
> >+ attrib_info *suffix = (attrib_info *) remove->elts;
> >+ int i;
> >+ for (i = 0; i < remove->nelts; i++) {
> >+ extension_info *exinfo =
> >+ (extension_info*)apr_hash_get(mappings,
> >+ suffix[i].name,
> >+ APR_HASH_KEY_STRING);
> >+ /* If we find the entry, the given member isn't NULL,
> >+ * and this exinfo wasn't allocated from the current pool,
> >+ * then we must copy before we remove the given member.
> >+ */
> >
> Why is it necessary to make a copy before overwriting the
> field in exinfo with NULL?
The hash members are _not_ copied by the apr_hash functions! This is why the
original patch had broken Apache.
The hash member has 5 pointers. This function nullifies one of them. If you don't
copy first, you are actually wiping out the registered value of the original config.
Worse, (in the overlay case) if you replace one of those pointers due to an .htaccess
override, you have just pointed into the r-> pool, so once this request is released,
the next request would segfault the server.
> >
> >+ if (!base->extension_mappings && !add->extension_mappings)
> >+ new->extension_mappings = NULL;
> >+ else if (base->extension_mappings
> >+ && ((base->pool == p) || !(add->extension_mappings
> >+ || add->handlers_remove
> >+ || add->types_remove
> >+ || add->encodings_remove)))
> >+ new->extension_mappings = base->extension_mappings;
> >+ else {
> >+ new->extension_mappings = apr_hash_make(p);
> >+ if (base->extension_mappings)
> > overlay_extension_mappings(p, base->extension_mappings,
> > new->extension_mappings);
> >+ }
> >
> One minor problem here (and I can't remember if it will be
> a problem in practice): this logic can call overlay_extension_mappings
> with one of its latter two arguments equal to NULL, and
> overlay_extension_mappings
> will dereference NULL without checking.
That's why we test first. new->extension_mappings is guarenteed to be non-null.
Bill
Re: [PATCH] solves the mod_mime seg fault without loosing the hash?
Posted by Brian Pane <bp...@pacbell.net>.
William A. Rowe, Jr. wrote:
[...]
>+static void remove_items(apr_pool_t *p, apr_array_header_t *remove,
>+ apr_hash_t *mappings, apr_size_t member)
>+{
>+ attrib_info *suffix = (attrib_info *) remove->elts;
>+ int i;
>+ for (i = 0; i < remove->nelts; i++) {
>+ extension_info *exinfo =
>+ (extension_info*)apr_hash_get(mappings,
>+ suffix[i].name,
>+ APR_HASH_KEY_STRING);
>+ /* If we find the entry, the given member isn't NULL,
>+ * and this exinfo wasn't allocated from the current pool,
>+ * then we must copy before we remove the given member.
>+ */
>
Why is it necessary to make a copy before overwriting the
field in exinfo with NULL?
[...]
>
>+ if (!base->extension_mappings && !add->extension_mappings)
>+ new->extension_mappings = NULL;
>+ else if (base->extension_mappings
>+ && ((base->pool == p) || !(add->extension_mappings
>+ || add->handlers_remove
>+ || add->types_remove
>+ || add->encodings_remove)))
>+ new->extension_mappings = base->extension_mappings;
>+ else {
>+ new->extension_mappings = apr_hash_make(p);
>+ if (base->extension_mappings)
> overlay_extension_mappings(p, base->extension_mappings,
> new->extension_mappings);
>+ }
>
One minor problem here (and I can't remember if it will be
a problem in practice): this logic can call overlay_extension_mappings
with one of its latter two arguments equal to NULL, and
overlay_extension_mappings
will dereference NULL without checking.
--Brian
Re: [PATCH] solves the mod_mime seg fault without loosing the hash?
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "William A. Rowe, Jr." <wr...@rowe-clan.net>
Sent: Tuesday, August 14, 2001 9:51 PM
> > OK. More complex, and not 'tweaked' for legibility. Rather than make you all wait,
> > I'm posting as-is, and won't commit yet, since we have some contention about the 'right'
> > solution.
>
> Also introduces or reveals the segfault for me... I'm reviewing now, you might want to
> wait just a bit, I'll post a revision in an hour or so.
Sorry. As usual (when you hide things in nasty casts) it pays to test.
Attached is the proper patch. Please, test this against the segfault cases.
Bill
Re: [PATCH] solves the mod_mime seg fault without loosing the hash?
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "William A. Rowe, Jr." <wr...@rowe-clan.net>
Sent: Tuesday, August 14, 2001 9:39 PM
> OK. More complex, and not 'tweaked' for legibility. Rather than make you all wait,
> I'm posting as-is, and won't commit yet, since we have some contention about the 'right'
> solution.
Also introduces or reveals the segfault for me... I'm reviewing now, you might want to
wait just a bit, I'll post a revision in an hour or so.
Bill
[PATCH] solves the mod_mime seg fault without loosing the hash?
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Bill Stoddard" <bi...@wstoddard.com>
Sent: Tuesday, August 14, 2001 4:26 PM
> > This is getting obscene. We've lost all value of tables, since they can't be memcpy'ed,
> > they have to be reconstructed. This will be much slower than the original array code.
The option, Bill, to eliminate the hash entirely, is still in the realm of consideration.
But that patch can't undo Roy's and my bug fixes (that weren't related to the hash.)
> > I'll demonstrate a solution, using pool scopes, that adds safety and speed. Give me a day.
>
> Hit send before I was finished. If pool scopes significantly increases the complexity of
> this code, I will likely veto it. So you might want to share what you are planning on
> doing before you invest much time in it. I learned a lesson working on this code... the
> dir_create() and dir_merge() code needs to be as straight forward and easy to understand
> as possible to keep bugs out. Gaining 5% performance at the expense of a 100% increase in
> code complexity is not an acceptable trade off. The copy_mappings, et. al. was more like a
> 1000% increase in complexity.
OK. More complex, and not 'tweaked' for legibility. Rather than make you all wait,
I'm posting as-is, and won't commit yet, since we have some contention about the 'right'
solution.
The list of 'optimizable cases' shrinks in this patch. ->copy/copy_mappings is gone.
We only care that the cfg->pool and entry->pool are in-scope. If they aren't, we copy.
We've got to copy when we merge or remove. OH ... this patch defers the forced
'hash_make' until we actually add an entry. You may want to ignore that change (set
new->extension_mappings = apr_hash_make(p); in line 152) to continue to reproduce the
segfault reliably.
remove-items is kluged to eliminate a ton of lines (three reps). Less prone to error,
I hope. The big headache is the 'member' index into the exinfo structure. That 'offset'
code gets ugly, perhaps someone has a better/cleaner solution (using a macro or some such.)
Anyways, I've wrapped my brain around this another three hours. Try this patch if you can
reproduce the segfault (I can't, but couldn't before) and report back.
Bill
Re: [PATCH] I prefer this... Re: [PATCH] for mod_mime seg fault in 2.0.23 :-)
Posted by Bill Stoddard <bi...@wstoddard.com>.
> -1 (veto) to both suggestions.
>
> This is getting obscene. We've lost all value of tables, since they can't be memcpy'ed,
> they have to be reconstructed. This will be much slower than the original array code.
>
> Committers, please review dir_create() and dir_merge() patches more carefully. There
> is a long tradition of configuration bugs due to these assumptions. The patches that
> added copy/copy_mappings was an attempt to salvage this.
>
> I'll demonstrate a solution, using pool scopes, that adds safety and speed. Give me a
day.
>
Hit send before I was finished. If pool scopes significantly increases the complexity of
this code, I will likely veto it. So you might want to share what you are planning on
doing before you invest much time in it. I learned a lesson working on this code... the
dir_create() and dir_merge() code needs to be as straight forward and easy to understand
as possible to keep bugs out. Gaining 5% performance at the expense of a 100% increase in
code complexity is not an acceptable trade off. The copy_mappings, et. al. was more like a
1000% increase in complexity.
Bill
Re: [PATCH] I prefer this... Re: [PATCH] for mod_mime seg fault in 2.0.23 :-)
Posted by "Roy T. Fielding" <fi...@ebuilt.com>.
The sensible thing would be to stop putting so many implementation
assumptions throughout the code. If we separated the act of checking
the value from the nature of the config data structure, then we could
have look-up routines that acted on a list of layered configs instead
of copying each layer onto another onto another for every request.
That would speed up the high-performance case of no-overrides and
eliminate mixing of pools.
In the mean time, though, let's just fix the seg fault.
....Roy
Re: [PATCH] I prefer this... Re: [PATCH] for mod_mime seg fault in 2.0.23 :-)
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Brian Pane" <bp...@pacbell.net>
Sent: Tuesday, August 14, 2001 5:37 PM
> Are you still considering optimizing directory_walk so that it
> doesn't have to do all the dir_merge work on each request (either
> through a caching solution or through some variant of the pre-merge
> patch that I posted a while back)?
No, I think we _do_ want some pre-merge cache. That doesn't oblivate the
need to optimize one of the most top-heavy aspects of Apache modules.
Unless you are hiding all your Alias'es at a root directory, there can
be alot of this going on.
> If we didn't have to call merge_mime_dir_config with such alarming
> frequency, it might not be necessary to optimize this function (or
> dir_merge functions in general).
I still think optimizing dir merges is a defacto requirement. We just
have to do it gracefully (by pool scope, or whatever.)
Bill
Re: [PATCH] I prefer this... Re: [PATCH] for mod_mime seg fault in
2.0.23 :-)
Posted by Brian Pane <bp...@pacbell.net>.
William A. Rowe, Jr. wrote:
>-1 (veto) to both suggestions.
>
>This is getting obscene. We've lost all value of tables, since they can't be memcpy'ed,
>they have to be reconstructed. This will be much slower than the original array code.
>
>Committers, please review dir_create() and dir_merge() patches more carefully. There
>is a long tradition of configuration bugs due to these assumptions. The patches that
>added copy/copy_mappings was an attempt to salvage this.
>
>I'll demonstrate a solution, using pool scopes, that adds safety and speed. Give me a day.
>
Are you still considering optimizing directory_walk so that it
doesn't have to do all the dir_merge work on each request (either
through a caching solution or through some variant of the pre-merge
patch that I posted a while back)?
If we didn't have to call merge_mime_dir_config with such alarming
frequency, it might not be necessary to optimize this function (or
dir_merge functions in general).
--Brian
Re: [PATCH] I prefer this... Re: [PATCH] for mod_mime seg fault in 2.0.23 :-)
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
-1 (veto) to both suggestions.
This is getting obscene. We've lost all value of tables, since they can't be memcpy'ed,
they have to be reconstructed. This will be much slower than the original array code.
Committers, please review dir_create() and dir_merge() patches more carefully. There
is a long tradition of configuration bugs due to these assumptions. The patches that
added copy/copy_mappings was an attempt to salvage this.
I'll demonstrate a solution, using pool scopes, that adds safety and speed. Give me a day.
Bill
----- Original Message -----
From: "Bill Stoddard" <bi...@wstoddard.com>
To: <ne...@apache.org>
Sent: Tuesday, August 14, 2001 3:57 PM
Subject: [PATCH] I prefer this... Re: [PATCH] for mod_mime seg fault in 2.0.23 :-)
> I prefer this patch which creates a completely new, fully merged, mime_dir_config on each
> call to merge_mime_dir_configs. It is a bit more expensive but should it still work better
> than the table code (maybe. I'd like to see it profiled :-). And this code is a hell of a
> lot easier to understand :-)
>
> Bill
>
> Index: mod_mime.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/http/mod_mime.c,v
> retrieving revision 1.50
> diff -u -r1.50 mod_mime.c
> --- mod_mime.c 2001/08/14 02:35:55 1.50
> +++ mod_mime.c 2001/08/14 20:46:56
> @@ -105,14 +105,11 @@
> char *language_type; /* Added with AddLanguage... */
> char *handler; /* Added with AddHandler... */
> char *charset_type; /* Added with AddCharset... */
> - int copy; /* To avoid dupping in the merge */
> } extension_info;
>
> typedef struct {
> apr_hash_t *extension_mappings; /* Map from extension name to
> * extension_info structure */
> - int copy_mappings; /* To avoid dupping in the merge */
> -
> apr_array_header_t *handlers_remove; /* List of handlers to remove */
> apr_array_header_t *types_remove; /* List of MIME types to remove */
> apr_array_header_t *encodings_remove; /* List of encodings to remove */
> @@ -150,7 +147,6 @@
> (mime_dir_config *) apr_palloc(p, sizeof(mime_dir_config));
>
> new->extension_mappings = apr_hash_make(p);
> - new->copy_mappings = 0;
>
> new->handlers_remove = NULL;
> new->types_remove = NULL;
> @@ -180,13 +176,10 @@
> base_info = (extension_info*)apr_hash_get(base, key, klen);
>
> if (base_info) {
> - if (!base_info->copy) {
> - extension_info *copyinfo = base_info;
> - base_info = (extension_info*)apr_palloc(p, sizeof(*base_info));
> - apr_hash_set(base, key, klen, base_info);
> - memcpy(base_info, copyinfo, sizeof(*base_info));
> - base_info->copy = 1;
> - }
> + extension_info *copyinfo = base_info;
> + base_info = (extension_info*)apr_palloc(p, sizeof(*base_info));
> + apr_hash_set(base, key, klen, base_info);
> + memcpy(base_info, copyinfo, sizeof(*base_info));
>
> if (overlay_info->forced_type) {
> base_info->forced_type = overlay_info->forced_type;
> @@ -219,36 +212,15 @@
> int i;
> attrib_info *suffix;
>
> - if (base->extension_mappings && add->extension_mappings) {
> - if (base->copy_mappings)
> - new->extension_mappings = base->extension_mappings;
> - else {
> - new->extension_mappings = apr_hash_make(p);
> - overlay_extension_mappings(p, base->extension_mappings,
> - new->extension_mappings);
> - }
> - overlay_extension_mappings(p, add->extension_mappings,
> - new->extension_mappings);
> - new->copy_mappings = 1;
> + new->extension_mappings = apr_hash_make(p);
> +
> + if (base->extension_mappings != NULL) {
> + overlay_extension_mappings(p, base->extension_mappings,
> + new->extension_mappings);
> }
> - else {
> - if (base->extension_mappings == NULL) {
> - new->extension_mappings = add->extension_mappings;
> - new->copy_mappings = add->copy_mappings;
> - }
> - else if (add->extension_mappings == NULL) {
> - new->extension_mappings = base->extension_mappings;
> - new->copy_mappings = base->copy_mappings;
> - }
> - if (!new->copy_mappings && (add->handlers_remove
> - || add->types_remove
> - || add->encodings_remove)) {
> - apr_hash_t *copyhash = new->extension_mappings;
> - new->extension_mappings = apr_hash_make(p);
> - /* ### as slow as can be... just use an apr_hash_dup! */
> - overlay_extension_mappings(p, copyhash, new->extension_mappings);
> - new->copy_mappings = 1;
> - }
> + if (add->extension_mappings != NULL) {
> + overlay_extension_mappings(p, add->extension_mappings,
> + new->extension_mappings);
> }
>
> if (add->handlers_remove) {
> @@ -259,14 +231,6 @@
> suffix[i].name,
> APR_HASH_KEY_STRING);
> if (exinfo) {
> - if (!exinfo->copy) {
> - extension_info *copyinfo = exinfo;
> - exinfo = (extension_info*)apr_palloc(p, sizeof(*exinfo));
> - apr_hash_set(new->extension_mappings, suffix[i].name,
> - APR_HASH_KEY_STRING, exinfo);
> - memcpy(exinfo, copyinfo, sizeof(*exinfo));
> - exinfo->copy = 1;
> - }
> exinfo->handler = NULL;
> }
> }
> @@ -281,14 +245,6 @@
> suffix[i].name,
> APR_HASH_KEY_STRING);
> if (exinfo) {
> - if (!exinfo->copy) {
> - extension_info *copyinfo = exinfo;
> - exinfo = (extension_info*)apr_palloc(p, sizeof(*exinfo));
> - apr_hash_set(new->extension_mappings, suffix[i].name,
> - APR_HASH_KEY_STRING, exinfo);
> - memcpy(exinfo, copyinfo, sizeof(*exinfo));
> - exinfo->copy = 1;
> - }
> exinfo->forced_type = NULL;
> }
> }
> @@ -303,14 +259,6 @@
> suffix[i].name,
> APR_HASH_KEY_STRING);
> if (exinfo) {
> - if (!exinfo->copy) {
> - extension_info *copyinfo = exinfo;
> - exinfo = (extension_info*)apr_palloc(p, sizeof(*exinfo));
> - apr_hash_set(new->extension_mappings, suffix[i].name,
> - APR_HASH_KEY_STRING, exinfo);
> - memcpy(exinfo, copyinfo, sizeof(*exinfo));
> - exinfo->copy = 1;
> - }
> exinfo->encoding_type = NULL;
> }
> }
> @@ -792,6 +740,7 @@
> char *ext;
> const char *fn, *type, *charset = NULL;
> int found_metadata = 0;
> + extension_info *exinfo = NULL;
>
> if (r->finfo.filetype == APR_DIR) {
> r->content_type = DIR_MAGIC_TYPE;
> @@ -820,7 +769,7 @@
> /* Parse filename extensions which can be in any order
> */
> while (*fn && (ext = ap_getword(r->pool, &fn, '.'))) {
> - extension_info *exinfo;
> +
> int found;
>
> if (*ext == '\0') /* ignore empty extensions "bad..html" */
>
>
> ----- Original Message -----
> From: "Greg Ames" <gr...@remulak.net>
> To: <ne...@apache.org>
> Sent: Tuesday, August 14, 2001 3:50 PM
> Subject: [PATCH] for mod_mime seg fault in 2.0.23 :-)
>
>
> > Bill Stoddard wrote:
> > >
> >
> > > The problem is that we are creating an extension_info out of a subrequest pool and
> > > sticking it into a longer lived hash table (allocated out of the request pool I'm
> > > guessing). When we pull the extension_info out of the hash table later and try to use
> it,
> > > we seg fault because the storage has been reused for something else. No thoughts on a
> fix.
> >
> > This fixes the seg fault on my Linux box:
> >
> > Index: modules/http/mod_mime.c
> > ===================================================================
> > RCS file: /home/cvs/httpd-2.0/modules/http/mod_mime.c,v
> > retrieving revision 1.50
> > diff -u -d -b -r1.50 mod_mime.c
> > --- modules/http/mod_mime.c 2001/08/14 02:35:55 1.50
> > +++ modules/http/mod_mime.c 2001/08/14 19:39:45
> > @@ -220,13 +220,10 @@
> > attrib_info *suffix;
> >
> > if (base->extension_mappings && add->extension_mappings) {
> > - if (base->copy_mappings)
> > - new->extension_mappings = base->extension_mappings;
> > - else {
> > new->extension_mappings = apr_hash_make(p);
> > overlay_extension_mappings(p, base->extension_mappings,
> > new->extension_mappings);
> > - }
> > +
> > overlay_extension_mappings(p, add->extension_mappings,
> > new->extension_mappings);
> >
> > We can't allow overlay_extension_mappings to store things into the
> > extension_mappings hash table which have a shorter lifetime than the
> > hash table itself. Always using a new extension_mappings prevents this
> > from happening.
> >
> > I'll commit this in a little while unless someone objects soon.
> >
> > Greg
> > Greg
> >
>
>
[PATCH] I prefer this... Re: [PATCH] for mod_mime seg fault in 2.0.23 :-)
Posted by Bill Stoddard <bi...@wstoddard.com>.
I prefer this patch which creates a completely new, fully merged, mime_dir_config on each
call to merge_mime_dir_configs. It is a bit more expensive but should it still work better
than the table code (maybe. I'd like to see it profiled :-). And this code is a hell of a
lot easier to understand :-)
Bill
Index: mod_mime.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/mod_mime.c,v
retrieving revision 1.50
diff -u -r1.50 mod_mime.c
--- mod_mime.c 2001/08/14 02:35:55 1.50
+++ mod_mime.c 2001/08/14 20:46:56
@@ -105,14 +105,11 @@
char *language_type; /* Added with AddLanguage... */
char *handler; /* Added with AddHandler... */
char *charset_type; /* Added with AddCharset... */
- int copy; /* To avoid dupping in the merge */
} extension_info;
typedef struct {
apr_hash_t *extension_mappings; /* Map from extension name to
* extension_info structure */
- int copy_mappings; /* To avoid dupping in the merge */
-
apr_array_header_t *handlers_remove; /* List of handlers to remove */
apr_array_header_t *types_remove; /* List of MIME types to remove */
apr_array_header_t *encodings_remove; /* List of encodings to remove */
@@ -150,7 +147,6 @@
(mime_dir_config *) apr_palloc(p, sizeof(mime_dir_config));
new->extension_mappings = apr_hash_make(p);
- new->copy_mappings = 0;
new->handlers_remove = NULL;
new->types_remove = NULL;
@@ -180,13 +176,10 @@
base_info = (extension_info*)apr_hash_get(base, key, klen);
if (base_info) {
- if (!base_info->copy) {
- extension_info *copyinfo = base_info;
- base_info = (extension_info*)apr_palloc(p, sizeof(*base_info));
- apr_hash_set(base, key, klen, base_info);
- memcpy(base_info, copyinfo, sizeof(*base_info));
- base_info->copy = 1;
- }
+ extension_info *copyinfo = base_info;
+ base_info = (extension_info*)apr_palloc(p, sizeof(*base_info));
+ apr_hash_set(base, key, klen, base_info);
+ memcpy(base_info, copyinfo, sizeof(*base_info));
if (overlay_info->forced_type) {
base_info->forced_type = overlay_info->forced_type;
@@ -219,36 +212,15 @@
int i;
attrib_info *suffix;
- if (base->extension_mappings && add->extension_mappings) {
- if (base->copy_mappings)
- new->extension_mappings = base->extension_mappings;
- else {
- new->extension_mappings = apr_hash_make(p);
- overlay_extension_mappings(p, base->extension_mappings,
- new->extension_mappings);
- }
- overlay_extension_mappings(p, add->extension_mappings,
- new->extension_mappings);
- new->copy_mappings = 1;
+ new->extension_mappings = apr_hash_make(p);
+
+ if (base->extension_mappings != NULL) {
+ overlay_extension_mappings(p, base->extension_mappings,
+ new->extension_mappings);
}
- else {
- if (base->extension_mappings == NULL) {
- new->extension_mappings = add->extension_mappings;
- new->copy_mappings = add->copy_mappings;
- }
- else if (add->extension_mappings == NULL) {
- new->extension_mappings = base->extension_mappings;
- new->copy_mappings = base->copy_mappings;
- }
- if (!new->copy_mappings && (add->handlers_remove
- || add->types_remove
- || add->encodings_remove)) {
- apr_hash_t *copyhash = new->extension_mappings;
- new->extension_mappings = apr_hash_make(p);
- /* ### as slow as can be... just use an apr_hash_dup! */
- overlay_extension_mappings(p, copyhash, new->extension_mappings);
- new->copy_mappings = 1;
- }
+ if (add->extension_mappings != NULL) {
+ overlay_extension_mappings(p, add->extension_mappings,
+ new->extension_mappings);
}
if (add->handlers_remove) {
@@ -259,14 +231,6 @@
suffix[i].name,
APR_HASH_KEY_STRING);
if (exinfo) {
- if (!exinfo->copy) {
- extension_info *copyinfo = exinfo;
- exinfo = (extension_info*)apr_palloc(p, sizeof(*exinfo));
- apr_hash_set(new->extension_mappings, suffix[i].name,
- APR_HASH_KEY_STRING, exinfo);
- memcpy(exinfo, copyinfo, sizeof(*exinfo));
- exinfo->copy = 1;
- }
exinfo->handler = NULL;
}
}
@@ -281,14 +245,6 @@
suffix[i].name,
APR_HASH_KEY_STRING);
if (exinfo) {
- if (!exinfo->copy) {
- extension_info *copyinfo = exinfo;
- exinfo = (extension_info*)apr_palloc(p, sizeof(*exinfo));
- apr_hash_set(new->extension_mappings, suffix[i].name,
- APR_HASH_KEY_STRING, exinfo);
- memcpy(exinfo, copyinfo, sizeof(*exinfo));
- exinfo->copy = 1;
- }
exinfo->forced_type = NULL;
}
}
@@ -303,14 +259,6 @@
suffix[i].name,
APR_HASH_KEY_STRING);
if (exinfo) {
- if (!exinfo->copy) {
- extension_info *copyinfo = exinfo;
- exinfo = (extension_info*)apr_palloc(p, sizeof(*exinfo));
- apr_hash_set(new->extension_mappings, suffix[i].name,
- APR_HASH_KEY_STRING, exinfo);
- memcpy(exinfo, copyinfo, sizeof(*exinfo));
- exinfo->copy = 1;
- }
exinfo->encoding_type = NULL;
}
}
@@ -792,6 +740,7 @@
char *ext;
const char *fn, *type, *charset = NULL;
int found_metadata = 0;
+ extension_info *exinfo = NULL;
if (r->finfo.filetype == APR_DIR) {
r->content_type = DIR_MAGIC_TYPE;
@@ -820,7 +769,7 @@
/* Parse filename extensions which can be in any order
*/
while (*fn && (ext = ap_getword(r->pool, &fn, '.'))) {
- extension_info *exinfo;
+
int found;
if (*ext == '\0') /* ignore empty extensions "bad..html" */
----- Original Message -----
From: "Greg Ames" <gr...@remulak.net>
To: <ne...@apache.org>
Sent: Tuesday, August 14, 2001 3:50 PM
Subject: [PATCH] for mod_mime seg fault in 2.0.23 :-)
> Bill Stoddard wrote:
> >
>
> > The problem is that we are creating an extension_info out of a subrequest pool and
> > sticking it into a longer lived hash table (allocated out of the request pool I'm
> > guessing). When we pull the extension_info out of the hash table later and try to use
it,
> > we seg fault because the storage has been reused for something else. No thoughts on a
fix.
>
> This fixes the seg fault on my Linux box:
>
> Index: modules/http/mod_mime.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/http/mod_mime.c,v
> retrieving revision 1.50
> diff -u -d -b -r1.50 mod_mime.c
> --- modules/http/mod_mime.c 2001/08/14 02:35:55 1.50
> +++ modules/http/mod_mime.c 2001/08/14 19:39:45
> @@ -220,13 +220,10 @@
> attrib_info *suffix;
>
> if (base->extension_mappings && add->extension_mappings) {
> - if (base->copy_mappings)
> - new->extension_mappings = base->extension_mappings;
> - else {
> new->extension_mappings = apr_hash_make(p);
> overlay_extension_mappings(p, base->extension_mappings,
> new->extension_mappings);
> - }
> +
> overlay_extension_mappings(p, add->extension_mappings,
> new->extension_mappings);
>
> We can't allow overlay_extension_mappings to store things into the
> extension_mappings hash table which have a shorter lifetime than the
> hash table itself. Always using a new extension_mappings prevents this
> from happening.
>
> I'll commit this in a little while unless someone objects soon.
>
> Greg
> Greg
>
Re: [PATCH] for mod_mime seg fault in 2.0.23 :-)
Posted by Jeff Trawick <tr...@attglobal.net>.
Greg Ames <gr...@remulak.net> writes:
> This fixes the seg fault on my Linux box:
>
> Index: modules/http/mod_mime.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/http/mod_mime.c,v
> retrieving revision 1.50
> diff -u -d -b -r1.50 mod_mime.c
> --- modules/http/mod_mime.c 2001/08/14 02:35:55 1.50
> +++ modules/http/mod_mime.c 2001/08/14 19:39:45
> @@ -220,13 +220,10 @@
> attrib_info *suffix;
>
> if (base->extension_mappings && add->extension_mappings) {
> - if (base->copy_mappings)
> - new->extension_mappings = base->extension_mappings;
> - else {
> new->extension_mappings = apr_hash_make(p);
> overlay_extension_mappings(p, base->extension_mappings,
> new->extension_mappings);
> - }
> +
> overlay_extension_mappings(p, add->extension_mappings,
> new->extension_mappings);
I suggest committing it. Sure, Bill**2 are suggesting more extensive
changes but this works in the meantime. Even if "in the meantime"
isn't very long, we know that what was in CVS before the
more-extensive changes was working; also, reviewing the more extensive
changes will be a comparison of working with working instead of
working with broken, for what that is worth.
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...