-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" <[EMAIL PROTECTED]>
To: <[EMAIL PROTECTED]>
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" <[EMAIL PROTECTED]>
> To: <[EMAIL PROTECTED]>
> 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
> >
>
>
