apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: cvs commit: apr/file_io/unix filepath.c Makefile.in
Date Sat, 31 Mar 2001 21:27:25 GMT
On Sat, Mar 31, 2001 at 06:22:38AM -0000, wrowe@apache.org wrote:
>...
>   +/**
>   + * Extract the rootpath from the given filepath
>   + * @param rootpath the root file path returned with APR_SUCCESS or APR_EINCOMPLETE
>   + * @param filepath the pathname to parse for it's root component
>   + * @param p the pool to allocate the new path string from
>   + * @deffunc apr_status_t apr_filepath_root(const char **rootpath, const char **inpath,
apr_pool_t *p)
>   + * @tip on return, filepath now points to the character following the root.
>   + * In the simplest example, given a filepath of "/foo", returns the rootpath
>   + * of "/" and filepath points at "foo".  This is far more complex on other 
>   + * platforms, which even changes alternate format of rootpath to canonical
>   + * form.  The function returns APR_ERELATIVE if filepath isn't rooted (an
>   + * error), APR_EINCOMPLETE if the root path is ambigious (but potentially
>   + * legitimate, e.g. "/" on Windows is incomplete because it doesn't specify
>   + * the drive letter), or APR_EBADPATH if the root is simply invalid.
>   + * APR_SUCCESS is returned if filepath is an absolute path.
>   + */

I don't think it should ever return APR_EINCOMPLETE. If there is no drive
letter, then it should just return APR_ERELATIVE.

And don't say "current drive" because then you could say "current
directory." There is no effective difference between them.

>...
>   APR_DECLARE(apr_status_t) apr_filepath_parse_root(const char **rootpath, 
>                                                     const char **inpath,
>                                                     apr_pool_t *p)
>   {
>       if (**inpath == '/') 
>       {
>           *rootpath = apr_pstrdup(p, "/");
>           ++*inpath;
>           return APR_EABSOLUTE;
>       }
>   
>       return APR_ERELATIVE;
>   }

Euh... I expected rootpath to be "/foo/bar/" if it was passed "/foo/bar/baz"
Is the above function just a first hack, or was the intent to only return a
single "/" ??

If it *is* just a first hack, then you MUST leave a comment in there.
Otherwise, you get people (like me) wondering wtf is going on in there.

>   APR_DECLARE(apr_status_t) apr_filepath_merge(char **newpath, 
>                                                const char *rootpath, 
>                                                const char *addpath, 
>                                                apr_int32_t flags,
>                                                apr_pool_t *p)
>   {
>       char path[APR_PATH_MAX];
>       apr_size_t rootlen; /* is the original rootpath len */
>       apr_size_t newseg;  /* is the path offset to the added path */
>       apr_size_t addseg;  /* is the path offset we are appending at */

"addseg" is more descriptive as "pathlen", the current path length.

>       apr_size_t endseg;  /* is the end of the current segment */

And this would be "seglen".

>       apr_status_t rv;
>   
>       /* Treat null as an empty path.
>        */
>       if (!addpath)
>           addpath = "";
>   
>       if (addpath[0] == '/') 
>       {
>           /* If addpath is rooted, then rootpath is unused.
>            * Ths violates any APR_FILEPATH_SECUREROOTTEST and
>            * APR_FILEPATH_NOTABSOLUTE flags specified.
>            */
>           if (flags & APR_FILEPATH_SECUREROOTTEST)
>               return APR_EABOVEROOT;
>           if (flags & APR_FILEPATH_NOTABSOLUTE)
>               return APR_EABSOLUTE;
>   
>           /* If APR_FILEPATH_NOTABOVEROOT wasn't specified,
>            * we won't test the root again, it's ignored.
>            * Waste no CPU retrieving the working path.
>            */
>           if (!rootpath && !(flags & APR_FILEPATH_NOTABOVEROOT))
>               rootpath = "";
>       }
>       else 
>       {
>           /* If APR_FILEPATH_NOTABSOLUTE is specified, the caller 
>            * requires a relative result.  If the rootpath is
>            * ommitted, we do not retrieve the working path,
>            * if rootpath was supplied as absolute then fail.
>            */
>           if (flags & APR_FILEPATH_NOTABSOLUTE) 
>           {
>               if (!rootpath)
>                   rootpath = "";
>               else if (rootpath[0] == '/')
>                   return APR_EABSOLUTE;
>           }
>       }
>   
>       if (!rootpath) 
>       {
>           /* Start with the current working path.  This is bass akwards,
>            * but required since the compiler (at least vc) doesn't like
>            * passing the address of a char const* for a char** arg.
>            */
>           char *getpath;
>           rv = apr_filepath_get(&getpath, p);
>           rootpath = getpath;
>           if (rv != APR_SUCCESS)
>               return errno;
>       
>           /* XXX: Any kernel subject to goofy, uncanonical results
>            * must run the rootpath against the user's given flags.
>            * Simplest would be a recursive call to apr_filepath_merge
>            * with an empty (not null) rootpath and addpath of the cwd.
>            */
>       }
>   
>       rootlen = strlen(rootpath);
>   
>       if (addpath[0] == '/') 
>       {
>           /* Ignore the given root path, strip off leading 
>            * '/'s to a single leading '/' from the addpath,
>            * and leave addpath at the first non-'/' character.
>            */
>           newseg = 0;
>           while (addpath[0] == '/')
>               ++addpath;
>           strcpy (path, "/");
>           addseg = 1;
>       }
>       else
>       {
>           /* If both paths are relative, fail early
>            */
>           if (rootpath[0] != '/' && (flags & APR_FILEPATH_NOTRELATIVE))
>                   return APR_ERELATIVE;
>   
>           /* Base the result path on the rootpath
>            */
>           newseg = rootlen;
>           if (rootlen >= sizeof(path))
>               return APR_ENAMETOOLONG;
>           strcpy(path, rootpath);

memcpy(path, rootpath, rootlen);
path[rootlen] = '/0';

Might be faster. Definitely faster if you don't need to null-term.

Hmm. Looking at the code, you don't need the null-term until the very end.
You could null-term when the while-loop exits. Until then, use memcpy()
everywhere for speed.

>           /* Always '/' terminate the given root path
>            */
>           if (newseg && path[newseg - 1] != '/') {
>               if (newseg + 1 >= sizeof(path))
>                   return APR_ENAMETOOLONG;
>               path[newseg++] = '/';
>               path[newseg] = '\0';
>           }
>           addseg = newseg;
>       }
>   
>       while (*addpath) 
>       {
>           /* Parse each segment, find the closing '/' 
>            */
>           endseg = 0;
>           while (addpath[endseg] && addpath[endseg] != '/')
>               ++endseg;
>   
>           if (endseg == 0 || (endseg == 1 && addpath[0] == '.')) 
>           {
>               /* noop segment (/ or ./) so skip it 
>                */
>           }
>           else if (endseg == 2 && addpath[0] == '.' && addpath[1] ==
'.') 
>           {
>               /* backpath (../) */
>               if (addseg == 1 && path[0] == '/') 
>               {
>                   /* Attempt to move above root.  Always die if the 
>                    * APR_FILEPATH_SECUREROOTTEST flag is specified.
>                    */
>                   if (flags & APR_FILEPATH_SECUREROOTTEST)
>                       return APR_EABOVEROOT;
>                   
>                   /* Otherwise this is simply a noop, above root is root.
>                    * Flag that rootpath was entirely replaced.
>                    */
>                   newseg = 0;
>               }
>               else if (addseg == 0 || (addseg >= 3 
>                                     && strcmp(path + addseg - 3, "../") == 0))

The above test isn't strong enough. It could misfire on "/foo../". You need
to check for "whatever/../" and "../".

>               {
>                   /* Path is already backpathed or empty, if the
>                    * APR_FILEPATH_SECUREROOTTEST.was given die now.
>                    */
>                   if (flags & APR_FILEPATH_SECUREROOTTEST)
>                       return APR_EABOVEROOT;
>   
>                   /* Otherwise append another backpath.
>                    */
>                   if (addseg + 3 >= sizeof(path))
>                       return APR_ENAMETOOLONG;
>                   strcpy(path + addseg, "../");

memcpy(path + addseg, "../", 3);

>                   addseg += 3;
>               }
>               else 
>               {
>                   /* otherwise crop the prior segment 
>                    */
>                   do {
>                       --addseg;
>                   } while (addseg && path[addseg - 1] != '/');
>                   path[addseg] = '\0';

Null-term not needed.

>               }
>   
>               /* Now test if we are above where we started and back up
>                * the newseg offset to reflect the added/altered path.
>                */
>               if (addseg < newseg) 
>               {
>                   if (flags & APR_FILEPATH_SECUREROOTTEST)
>                       return APR_EABOVEROOT;
>                   newseg = addseg;
>               }
>           }
>           else 
>           {
>               /* An actual segment, append it to the destination path
>                */
>               apr_size_t i = (addpath[endseg] != '\0');
>               if (addseg + endseg + i >= sizeof(path))
>                   return APR_ENAMETOOLONG;
>               strncpy(path + addseg, addpath, endseg + i);

strncpy() doesn't always null-terminate. In the above code, it definitely
does not. Therefore, you'd want to use:

memcpy(path + addseg, addpath, endseg + i);

and then after the next line:

>               addseg += endseg + i;

path[addseg] = '\0';

>           }
>   
>           /* Skip over trailing slash to the next segment
>            */
>           if (addpath[endseg])
>               ++endseg;
>   
>           addpath += endseg;
>       }
>   
>       /* newseg will be the rootlen unless the addpath contained
>        * backpath elements.  If so, and APR_FILEPATH_NOTABOVEROOT
>        * is specified (APR_FILEPATH_SECUREROOTTEST was caught above),
>        * compare the original root to assure the result path is
>        * still within given root path.
>        */
>       if ((flags & APR_FILEPATH_NOTABOVEROOT) && newseg < rootlen) {
>           if (strncmp(rootpath, path, rootlen))
>               return APR_EABOVEROOT;
>           if (rootpath[rootlen - 1] != '/'
>                   && path[rootlen] && path[rootlen] != '/')
>               return APR_EABOVEROOT;
>       }
>   
>       *newpath = apr_pstrdup(p, path);
>       return (newpath ? APR_SUCCESS : APR_ENOMEM);

newpath will always be != NULL, so this test is unneeded.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message