httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marc Slemko <>
Subject Re: mod_rewrite 3.0.0 for Apache 1.2b7 !!! (PATCH)
Date Sat, 01 Feb 1997 00:44:29 GMT
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.
> 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.


Why? 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)

+                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.  

+    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
+        rewritelog(r, 5, "setting env variable '%s' to '%s'", var, val);
+    }       

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

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

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.

+    /* The locking support:
+       Try to determine whether we should use
+       fcntl() or flock(). */ 
+#define USE_FCNTL 1 
+#include <fcntl.h>
+#define USE_FLOCK 1
+#include <sys/file.h>
+#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>

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

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.

Other than the above, looks good.

View raw message