httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From r..@engelschall.com (Ralf S. Engelschall)
Subject Re: mod_rewrite 3.0.0 for Apache 1.2b7 !!! (PATCH)
Date Sat, 01 Feb 1997 11:50:39 GMT

In article <Pine.BSF.3.95.970131173110.20358G-100000@alive.ampr.ab.ca> you wrote:
> On Fri, 31 Jan 1997, Ralf S. Engelschall wrote:

> > 
> > As a result of the current problems, I decided a change for mod_rewrite:
> > 
> > 1. Version 2.4.x (currently 2.4.1) is the latest release available
> >    for Apache 1.1.x. There will be only bugfixes applied to 2.4.x
> >    to provide a stable version for the Apache 1.1.x users.
> > 
> > 2. Version 3.0.0 was created by manually merging (hmm... I like
> >    those jobs) the 2.4.1 version and the aligned 2.3.10+ version currently
> >    found in the 1.2b7-dev snapshot sources. Actually this is 2.4.1 with Marcs
> >    snprintf/strncpy patches re-applied.  All other changes from the Apache
> >    Group (OS\2 EMX, const, etc.) were already included into 2.4.1.
> > 
> > Version 3.0.0 is still not released. Below is a standard patch for review
> > _AND_ a context diff to bring the current CVS sources up to date.
> > 
> > NOW, APACHE GROUP MEMBERS, ATTENTION PLEASE:
> > 
> > Go to the patch and review it. And do it immediately so it don't get lost in
> > the processing ;-) and Apache 1.2b7 can contain mod_rewrite 3.0.0. Please vote
> > for this change NOW!
> > 

> +1 once the following concerns are addressed by either convincing me I'm
> stupid or changing the source.  All the quotes below are from the
> output of a diff -u.

> -**      http://www.engelschall.com/
> +**      www.engelschall.com

> Why?  www.engelschall.com isn't a URL, or don't you want it to be. 
> (No, I will not object to the patch based on this but I just have
> this thing about so-called URLs, not that is is necessarily one)

It should not be an URL any longer. If you look at my signature you see that
my standpoint is: If one wants to reach or contact me, he should know what a
RFC822 address is and what a name www.xxx.com stands for. 

> +                for (i = 0; p->env[i] != NULL; i++) {
> +                    strcpy(env2, p->env[i]);
> +                    strcpy(env, pregsub(r->pool, env2, uri, regexp->re_nsub+1,
> regmatch));    /* substitute in output */
> +                    add_env_variable(r, env);
> +                }
>         
> Please use strncpy.  strcpy at that place is almost certainly safe
> today because of fake limits imposed on nearly all input, but that
> may change.  (this bit of code is duplicated 3 times).  If it isn't
> _extremely_ obvious that it is impossible for there to be an overflow (eg.
> one of my modifications that used strncpy(foo, "this") instead of strcpy),
> don't use strcpy.  

Ok, replaced all left strcpy now. And I substituted all ugly
buf[sizeof(buf]-1] = '\0' with a more descriptive EOS_PARANOIA(buf) macro ;-)

> +    if ((cp = strchr(s, ':')) != NULL) { 
> +        memcpy(var, s, cp-s);
> +        var[cp-s] = '\0';
> +        strcpy(val, cp+1);
> +        table_set(r->subprocess_env, pstrdup(r->pool, var), pstrdup(r->pool,
va
> l)); 
> +        rewritelog(r, 5, "setting env variable '%s' to '%s'", var, val);
> +    }       

> Check the memcpy to be sure (cp-s < sizeof(var)).  And the strcpy.

Fixed.

> +#ifdef USE_FCNTL
> +static struct flock   lock_it = { F_WRLCK, 0, 0, 0 };
> +static struct flock unlock_it = { F_UNLCK, 0, 0, 0 };
> +#endif

> Init it by naming the members explicitly.  I know this is done the
> same way in the accept_mutex stuff as you do it, but it is broken
> if you try to compile it on many platforms because the member order
> differs; that code currently isn't used on those platforms.  Using
> names _should_ be portable, but it will need to be tested.

Changes to name usage.

> +    /* The locking support:
> +       Try to determine whether we should use
> +       fcntl() or flock(). */ 
> +#if defined(USE_FCNTL_SERIALIZED_ACCEPT)
> +#define USE_FCNTL 1 
> +#include <fcntl.h>
> +#endif  
> +#if defined(USE_FLOCK_SERIALIZED_ACCEPT)
> +#define USE_FLOCK 1
> +#include <sys/file.h>
> +#endif
> +#if !defined(USE_FCNTL) && !defined(USE_FLOCK)
> +#define USE_FLOCK 1
> +#include <sys/file.h>
> +#ifndef LOCK_UN
> +#undef USE_FLOCK
> +#define USE_FCNTL 1
> +#include <fcntl.h>
> +#endif
> +#endif

> Perhaps it is worthwhile changing the server code to have USE_FCNTL and 
> USE_FLOCK defined elsewhere, and removing USE_FCNTL_SERIALIZED_ACCEPT and 
> USE_FLOCK_SERIALIZED_ACCEPT, replacing them with a USE_SERIALIZED_ACCEPT
> that checks USE_FCNTL and USE_FLOCK.  Actually, those should be
> HAVE_FCNTL and HAVE_FLOCK if in conf.h.  

YES! Pleaaaasse! I hate this determine stuff. What I currently couldn't do it
better. It would be very useful if conf.h contain USE_FCNTL or USE_FLOCK!
Please add such a check.

> I would also be wary of the portability of that method of testing
> which one to use.  Not that I have a better suggestion, but just something
> to keep in mind.

Hmmm.. this way is is 90% portable, except the situation where sys/file.h does
not exists for the default clause.

> Other than the above, looks good.

Fine.

Greetings,
                                       Ralf S. Engelschall
                                       rse@engelschall.com
                                       www.engelschall.com

Mime
View raw message