httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: [PATCH] I prefer this... Re: [PATCH] for mod_mime seg fault in 2.0.23 :-)
Date Tue, 14 Aug 2001 21:12:41 GMT
-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" <bill@wstoddard.com>
To: <new-httpd@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" <gregames@remulak.net>
> To: <new-httpd@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
> >
> 
> 


Mime
View raw message