You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "William A. Rowe, Jr." <wr...@rowe-clan.net> on 2001/02/10 18:12:42 UTC
[patch] again, mod_userdir
Sorry I've been rather invisible this week, caught a rather nasty head/chest cold,
and the return flight back from SFO to Chicago completely wasted me. I noticed there
has been no comment on this patch, and since we would all like to get a solid 1.3.18
out the door that -doesn't- have to be revisited later, I'd really appreciate some
code review and feedback. PR7211 indicates this is still a problem.
My regression testing of the patch included the following successful paths:
UserDir "d:\Documents and Settings\*\My Documents\My Website\"
UserDir "d:\apache\users\*\"
UserDir "d:\apache\users\"
UserDir "\apache\users\"
UserDir "http://www.rowe-clan.net/~*/"
On Win32 we fail (correctly) on this variant:
UserDir "public_html"
The code is refactored to make the code paths nearly legible. Nearly all the
platform discrepancies are gone. Here is the repost of that patch:
Index: mod_userdir.c
===================================================================
RCS file: /home/cvs/apache-1.3/src/modules/standard/mod_userdir.c,v
retrieving revision 1.48
diff -u -r1.48 mod_userdir.c
--- mod_userdir.c 2001/01/25 22:59:36 1.48
+++ mod_userdir.c 2001/02/10 17:07:45
@@ -207,7 +207,6 @@
const char *userdirs = s_cfg->userdir;
const char *w, *dname;
char *redirect;
- char *x = NULL;
struct stat statbuf;
/*
@@ -266,41 +265,58 @@
while (*userdirs) {
const char *userdir = ap_getword_conf(r->pool, &userdirs);
char *filename = NULL;
-#if defined(NETWARE) || defined(HAVE_DRIVE_LETTERS)
int is_absolute = ap_os_is_path_absolute(userdir);
-#endif
- if (strchr(userdir, '*'))
- x = ap_getword(r->pool, &userdir, '*');
-
- if (userdir[0] == '\0' || userdir[0] == '/') {
- if (x) {
-#if defined(NETWARE) || defined(HAVE_DRIVE_LETTERS)
- if (strchr(x, ':') && !is_absolute )
-#else /* !(NETWARE || HAVE_DRIVE_LETTERS) */
- if (strchr(x, ':'))
-#endif
- {
- redirect = ap_pstrcat(r->pool, x, w, userdir, dname, NULL);
- ap_table_setn(r->headers_out, "Location", redirect);
- return REDIRECT;
- }
- else
- filename = ap_pstrcat(r->pool, x, w, userdir, NULL);
+ if (strchr(userdir, '*')) {
+ /* token '*' embedded:
+ */
+ char *x = ap_getword(r->pool, &userdir, '*');
+ if (is_absolute) {
+ /* token '*' within absolute path
+ * serves [UserDir arg-pre*][user][UserDir arg-post*]
+ * /somepath/ * /somedir + /~smith -> /somepath/smith/somedir
+ */
+ filename = ap_pstrcat(r->pool, x, w, userdir, NULL);
+ }
+ else if (strchr(x, ':')) {
+ /* token '*' within a redirect path
+ * serves [UserDir arg-pre*][user][UserDir arg-post*]
+ * http://server/user/ * + /~smith/foo -> http://server/user/smith/foo
+ */
+ redirect = ap_pstrcat(r->pool, x, w, userdir, dname, NULL);
+ ap_table_setn(r->headers_out, "Location", redirect);
+ return REDIRECT;
}
- else
- filename = ap_pstrcat(r->pool, userdir, "/", w, NULL);
+ else {
+ /* Not a redirect, not an absolute path, '*' token:
+ * serves [homedir]/[UserDir arg]
+ * something/ * /public_html
+ */
+ /* Yuck, what's up with that? */
+ return DECLINED;
+ }
+ }
+ else if (is_absolute) {
+ /* An absolute path, no * token:
+ * serves [UserDir arg]/[user]
+ * /home + /~smith -> /home/smith
+ */
+ filename = ap_pstrcat(r->pool, userdir, "/", w, NULL);
}
-#if defined(NETWARE) || defined(HAVE_DRIVE_LETTERS)
- else if (strchr(userdir, ':') && !is_absolute ) {
-#else /* !(NETWARE || HAVE_DRIVE_LETTERS) */
else if (strchr(userdir, ':')) {
-#endif
+ /* A redirect, not an absolute path, no * token:
+ * serves [UserDir arg]/[user][dname]
+ * http://server/ + /~smith/foo -> http://server/smith/foo
+ */
redirect = ap_pstrcat(r->pool, userdir, "/", w, dname, NULL);
ap_table_setn(r->headers_out, "Location", redirect);
return REDIRECT;
}
else {
+ /* Not a redirect, not an absolute path, no * token:
+ * serves [homedir]/[UserDir arg]
+ * e.g. /~smith -> /home/smith/public_html
+ */
#if defined(WIN32) || defined(NETWARE)
/* Need to figure out home dirs on NT and NetWare */
return DECLINED;
Re: [patch] again, mod_userdir
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Greg Stein" <gs...@lyra.org>
Sent: Saturday, February 10, 2001 4:37 PM
> Seems fine, although I don't like returning DECLINED. That feels like
> "silent failure". Given that we could be subtly changing the semantics on
> some people's configurations (e.g. they relied on a broken form working),
> then I think it would be best to log (misconfigured) and error and return
> HTTP_INTERNAL_SERVER_ERROR.
>
> Even better would be to catch the error in the UserDir config command. In
> that case, right before each "return DECLINED", we could insert a comment
> stating /* shouldn't happen; erroneous form caugh in cmd_userdir */ (or
> whatever the func name is)
I agree that HTTP_INTERNAL_SERVER_ERROR better maps what we intended, since
we don't want this request to continue to walk through the handlers. This
is exactly how the MAX_PATH bug bit us. I'll make that change before
committing. Thanks for reviewing Greg, is anyone else out there?
Bill
Re: [patch] again, mod_userdir
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Greg Stein" <gs...@lyra.org>
Sent: Saturday, February 10, 2001 4:37 PM
> Seems fine, although I don't like returning DECLINED. That feels like
> "silent failure". Given that we could be subtly changing the semantics on
> some people's configurations (e.g. they relied on a broken form working),
> then I think it would be best to log (misconfigured) and error and return
> HTTP_INTERNAL_SERVER_ERROR.
>
> Even better would be to catch the error in the UserDir config command. In
> that case, right before each "return DECLINED", we could insert a comment
> stating /* shouldn't happen; erroneous form caugh in cmd_userdir */ (or
> whatever the func name is)
>
> The code below is simpler by virtue of looking for is_absolute before
> looking for ':' to mean a redirect. Good choice.
And good point... here's the updated patch. Two more folks actually reading
this patch would be good so I can get it committed :-)
Index: modules/standard/mod_userdir.c
===================================================================
RCS file: /home/cvs/apache-1.3/src/modules/standard/mod_userdir.c,v
retrieving revision 1.48
diff -u -r1.48 mod_userdir.c
--- modules/standard/mod_userdir.c 2001/01/25 22:59:36 1.48
+++ modules/standard/mod_userdir.c 2001/02/12 07:10:20
@@ -169,6 +169,14 @@
* If the first (only?) value isn't one of our keywords, just copy
* the string to the userdir string.
*/
+ if (!ap_os_is_path_absolute(arg) && !strchr(arg, ':'))
+#if defined(WIN32) || defined(NETWARE)
+ return "UserDir must specify an absolute redirect or absolute "
+ "file path";
+#else
+ if (strchr(arg, '*'))
+ return "UserDir cannot specify a both a relative path and '*' substitution"
+#endif
s_cfg->userdir = ap_pstrdup(cmd->pool, arg);
#if defined(WIN32) || defined(OS2) || defined(NETWARE)
/* This is an incomplete path, so we cannot canonicalize it yet.
@@ -207,7 +215,6 @@
const char *userdirs = s_cfg->userdir;
const char *w, *dname;
char *redirect;
- char *x = NULL;
struct stat statbuf;
/*
@@ -266,43 +273,62 @@
while (*userdirs) {
const char *userdir = ap_getword_conf(r->pool, &userdirs);
char *filename = NULL;
-#if defined(NETWARE) || defined(HAVE_DRIVE_LETTERS)
int is_absolute = ap_os_is_path_absolute(userdir);
-#endif
- if (strchr(userdir, '*'))
- x = ap_getword(r->pool, &userdir, '*');
-
- if (userdir[0] == '\0' || userdir[0] == '/') {
- if (x) {
-#if defined(NETWARE) || defined(HAVE_DRIVE_LETTERS)
- if (strchr(x, ':') && !is_absolute )
-#else /* !(NETWARE || HAVE_DRIVE_LETTERS) */
- if (strchr(x, ':'))
-#endif
- {
- redirect = ap_pstrcat(r->pool, x, w, userdir, dname, NULL);
- ap_table_setn(r->headers_out, "Location", redirect);
- return REDIRECT;
- }
- else
- filename = ap_pstrcat(r->pool, x, w, userdir, NULL);
+ if (strchr(userdir, '*')) {
+ /* token '*' embedded:
+ */
+ char *x = ap_getword(r->pool, &userdir, '*');
+ if (is_absolute) {
+ /* token '*' within absolute path
+ * serves [UserDir arg-pre*][user][UserDir arg-post*]
+ * /somepath/ * /somedir + /~smith -> /somepath/smith/somedir
+ */
+ filename = ap_pstrcat(r->pool, x, w, userdir, NULL);
}
- else
- filename = ap_pstrcat(r->pool, userdir, "/", w, NULL);
+ else if (strchr(x, ':')) {
+ /* token '*' within a redirect path
+ * serves [UserDir arg-pre*][user][UserDir arg-post*]
+ * http://server/user/ * + /~smith/foo -> http://server/user/smith/foo
+ */
+ redirect = ap_pstrcat(r->pool, x, w, userdir, dname, NULL);
+ ap_table_setn(r->headers_out, "Location", redirect);
+ return REDIRECT;
+ }
+ else {
+ /* Not a redirect, not an absolute path, '*' token:
+ * serves [homedir]/[UserDir arg]
+ * something/ * /public_html
+ * Shouldn't happen, we trap for this in set_user_dir
+ */
+ return DECLINED;
+ }
}
-#if defined(NETWARE) || defined(HAVE_DRIVE_LETTERS)
- else if (strchr(userdir, ':') && !is_absolute ) {
-#else /* !(NETWARE || HAVE_DRIVE_LETTERS) */
+ else if (is_absolute) {
+ /* An absolute path, no * token:
+ * serves [UserDir arg]/[user]
+ * /home + /~smith -> /home/smith
+ */
+ filename = ap_pstrcat(r->pool, userdir, "/", w, NULL);
+ }
else if (strchr(userdir, ':')) {
-#endif
+ /* A redirect, not an absolute path, no * token:
+ * serves [UserDir arg]/[user][dname]
+ * http://server/ + /~smith/foo -> http://server/smith/foo
+ */
redirect = ap_pstrcat(r->pool, userdir, "/", w, dname, NULL);
ap_table_setn(r->headers_out, "Location", redirect);
return REDIRECT;
}
else {
+ /* Not a redirect, not an absolute path, no * token:
+ * serves [homedir]/[UserDir arg]
+ * e.g. /~smith -> /home/smith/public_html
+ */
#if defined(WIN32) || defined(NETWARE)
- /* Need to figure out home dirs on NT and NetWare */
+ /* Need to figure out home dirs on NT and NetWare
+ * Shouldn't happen here, though, we trap for this in set_user_dir
+ */
return DECLINED;
#else /* WIN32 & NetWare */
struct passwd *pw;
Re: [patch] again, mod_userdir
Posted by Greg Stein <gs...@lyra.org>.
On Sat, Feb 10, 2001 at 11:12:42AM -0600, William A. Rowe, Jr. wrote:
> Sorry I've been rather invisible this week, caught a rather nasty head/chest cold,
> and the return flight back from SFO to Chicago completely wasted me. I noticed there
> has been no comment on this patch, and since we would all like to get a solid 1.3.18
> out the door that -doesn't- have to be revisited later, I'd really appreciate some
> code review and feedback. PR7211 indicates this is still a problem.
Seems fine, although I don't like returning DECLINED. That feels like
"silent failure". Given that we could be subtly changing the semantics on
some people's configurations (e.g. they relied on a broken form working),
then I think it would be best to log (misconfigured) and error and return
HTTP_INTERNAL_SERVER_ERROR.
Even better would be to catch the error in the UserDir config command. In
that case, right before each "return DECLINED", we could insert a comment
stating /* shouldn't happen; erroneous form caugh in cmd_userdir */ (or
whatever the func name is)
The code below is simpler by virtue of looking for is_absolute before
looking for ':' to mean a redirect. Good choice.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/