Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (asf.osuosl.org: local policy)
Message-ID: <44BE51DD.4070701@rowe-clan.net>
Date: Wed, 19 Jul 2006 10:38:05 -0500
From: "William A. Rowe, Jr." <wrowe@rowe-clan.net>
User-Agent: Thunderbird 1.5.0.4 (X11/20060614)
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Re: 2.0.x
References: <44BE29AA.8080901@apache.org> <44BE4C2A.50109@rowe-clan.net>
In-Reply-To: <44BE4C2A.50109@rowe-clan.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Actually, does this handle the nested case (more than one level depth?)

I'm afraid it doesn't.  Staying with the 'familiar' theme, it seems to
make sense to follow the entire code path, optimizing out the actual
operations by checking if (familiar).

William A. Rowe, Jr. wrote:
> This is much closer to what I'm working with, I like your patch better 
> in fact.
> 
> Please commit to trunk; I believe we can reoptimize the familiar case using
> a different rule based on a cached canonical filename pattern.  -but- this
> shouldn't hold up committing this fix; if you can save two flavors which
> apply clean to 2.0.58, 2.2.2, then we should make these available 
> immediately
> under /dist/httpd/patches/apply_to_2.x.x/ locations.
> 
> Working on a perl-framework validation that my collegue Sris started, 
> and will
> commit that the moment it's ready.
> 
> Bill
> 
> Ruediger Pluem wrote:
>> I have a patch that works against trunk (checked Options None, Options
>> FollowSymLinks, Options SymLinksifOwnerMatch). It keeps the 
>> optimization and
>> comes at the cost of one extra stat if we have FollowSymLinks NOT set 
>> or if
>> SymLinksifOwnerMatch is set and up to 3 further stats due to 
>> resolve_symlink if
>> it is really a symbolic link. From my current point of view this seems 
>> to be an
>> acceptable penalty. As I am not very familar with this code and this 
>> part of the
>> code seems to be critical from the performance and security 
>> perspective remote
>> eyes, reviews and comments are very welcome.
>>
>>
>> Index: server/request.c
>> ===================================================================
>> --- server/request.c    (Revision 422739)
>> +++ server/request.c    (Arbeitskopie)
>> @@ -524,17 +524,50 @@
>>                  && (!r->path_info || !*r->path_info)))
>>          && (cache->dir_conf_tested == sec_ent)
>>          && (strcmp(entry_dir, cache->cached) == 0)) {
>> +        int familar = 0;
>> +
>>          /* Well this looks really familiar!  If our end-result 
>> (per_dir_result)
>>           * didn't change, we have absolutely nothing to do :)
>>           * Otherwise (as is the case with most dir_merged/file_merged 
>> requests)
>>           * we must merge our dir_conf_merged onto this new 
>> r->per_dir_config.
>>           */
>>          if (r->per_dir_config == cache->per_dir_result) {
>> -            return OK;
>> +            familar = 1;
>>          }
>>
>>          if (r->per_dir_config == cache->dir_conf_merged) {
>>              r->per_dir_config = cache->per_dir_result;
>> +            familar = 1;
>> +        }
>> +
>> +        if (familar) {
>> +            apr_finfo_t thisinfo;
>> +            int res;
>> +            allow_options_t opts;
>> +            core_dir_config *this_dir;
>> +
>> +            this_dir = ap_get_module_config(r->per_dir_config, 
>> &core_module);
>> +            opts = this_dir->opts;
>> +            /*
>> +             * If Symlinks are allowed in general we do not need the 
>> following
>> +             * check.
>> +             */
>> +            if (!(opts & OPT_SYM_LINKS)) {
>> +                apr_stat(&thisinfo, r->filename,
>> +                         APR_FINFO_MIN | APR_FINFO_NAME | 
>> APR_FINFO_LINK,
>> +                         r->pool);
>> +                if (thisinfo.filetype == APR_LNK) {
>> +                    /* Is this a possibly acceptable symlink? */
>> +                    if ((res = resolve_symlink(r->filename, &thisinfo,
>> +                                               opts, r->pool)) != OK) {
>> +                        ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
>> +                                      "Symbolic link not allowed "
>> +                                      "or link target not accessible: 
>> %s",
>> +                                      r->filename);
>> +                        return r->status = res;
>> +                    }
>> +                }
>> +            }
>>              return OK;
>>          }
>>
>>
>>
>> Regards
>>
>> Rüdiger
>>
>>
>> ------------------------------------------------------------------------
>>
>> Index: server/request.c
>> ===================================================================
>> --- server/request.c    (Revision 422739)
>> +++ server/request.c    (Arbeitskopie)
>> @@ -524,17 +524,50 @@
>>                  && (!r->path_info || !*r->path_info)))
>>          && (cache->dir_conf_tested == sec_ent)
>>          && (strcmp(entry_dir, cache->cached) == 0)) {
>> +        int familar = 0;
>> +
>>          /* Well this looks really familiar!  If our end-result 
>> (per_dir_result)
>>           * didn't change, we have absolutely nothing to do :)
>>           * Otherwise (as is the case with most dir_merged/file_merged 
>> requests)
>>           * we must merge our dir_conf_merged onto this new 
>> r->per_dir_config.
>>           */
>>          if (r->per_dir_config == cache->per_dir_result) {
>> -            return OK;
>> +            familar = 1;
>>          }
>>  
>>          if (r->per_dir_config == cache->dir_conf_merged) {
>>              r->per_dir_config = cache->per_dir_result;
>> +            familar = 1;
>> +        }
>> +
>> +        if (familar) {
>> +            apr_finfo_t thisinfo;
>> +            int res;
>> +            allow_options_t opts;
>> +            core_dir_config *this_dir;
>> +
>> +            this_dir = ap_get_module_config(r->per_dir_config, 
>> &core_module);
>> +            opts = this_dir->opts;
>> +            /*
>> +             * If Symlinks are allowed in general we do not need the 
>> following
>> +             * check.
>> +             */
>> +            if (!(opts & OPT_SYM_LINKS)) {
>> +                apr_stat(&thisinfo, r->filename,
>> +                         APR_FINFO_MIN | APR_FINFO_NAME | 
>> APR_FINFO_LINK,
>> +                         r->pool);
>> +                if (thisinfo.filetype == APR_LNK) {
>> +                    /* Is this a possibly acceptable symlink? */
>> +                    if ((res = resolve_symlink(r->filename, &thisinfo,
>> +                                               opts, r->pool)) != OK) {
>> +                        ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
>> +                                      "Symbolic link not allowed "
>> +                                      "or link target not accessible: 
>> %s",
>> +                                      r->filename);
>> +                        return r->status = res;
>> +                    }
>> +                }
>> +            }
>>              return OK;
>>          }
>>  
> 
> 
>