You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by sf...@apache.org on 2012/09/14 23:06:06 UTC
svn commit: r1384924 - in /httpd/httpd/trunk: include/httpd.h
server/request.c
Author: sf
Date: Fri Sep 14 21:06:05 2012
New Revision: 1384924
URL: http://svn.apache.org/viewvc?rev=1384924&view=rev
Log:
ap_sub_req_lookup_dirent() depends on the over-allocation done by
ap_make_full_path and ap_escape_uri, so let's document it so that it is not
accidentally removed.
Modified:
httpd/httpd/trunk/include/httpd.h
httpd/httpd/trunk/server/request.c
Modified: httpd/httpd/trunk/include/httpd.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/httpd.h?rev=1384924&r1=1384923&r2=1384924&view=diff
==============================================================================
--- httpd/httpd/trunk/include/httpd.h (original)
+++ httpd/httpd/trunk/include/httpd.h Fri Sep 14 21:06:05 2012
@@ -1579,7 +1579,9 @@ AP_DECLARE(char *) ap_escape_path_segmen
* @param p The pool to allocate from
* @param path The path to convert
* @param partial if set, assume that the path will be appended to something
- * with a '/' in it (and thus does not prefix "./")
+ * with a '/' in it (and thus does not prefix "./").
+ * If not set, there will be one byte of additional space after the
+ * NUL, to allow the caller to append a '/'.
* @return The converted URL
*/
AP_DECLARE(char *) ap_os_escape_path(apr_pool_t *p, const char *path, int partial);
@@ -1692,7 +1694,8 @@ AP_DECLARE(char *) ap_make_dirstr_parent
* @param a The pool to allocate from
* @param dir The directory name
* @param f The filename
- * @return A copy of the full path
+ * @return A copy of the full path, with one byte of extra space after the NUL
+ * to allow the caller to add a trailing '/'.
* @note Never consider using this function if you are dealing with filesystem
* names that need to remain canonical, unless you are merging an apr_dir_read
* path and returned filename. Otherwise, the result is not canonical.
Modified: httpd/httpd/trunk/server/request.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/request.c?rev=1384924&r1=1384923&r2=1384924&view=diff
==============================================================================
--- httpd/httpd/trunk/server/request.c (original)
+++ httpd/httpd/trunk/server/request.c Fri Sep 14 21:06:05 2012
@@ -2160,7 +2160,7 @@ AP_DECLARE(request_rec *) ap_sub_req_loo
}
if (rnew->finfo.filetype == APR_DIR) {
- /* ap_make_full_path overallocated the buffers
+ /* ap_make_full_path and ap_escape_uri overallocated the buffers
* by one character to help us out here.
*/
strcat(rnew->filename, "/");
Re: svn commit: r1384924 - in /httpd/httpd/trunk: include/httpd.h server/request.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 24 Apr 2014, at 8:34 AM, Christophe JAILLET <ch...@wanadoo.fr> wrote:
> the comment is wrong.
>
> 'ap_sub_req_lookup_dirent' uses the fact that 'rnew->uri' has some extra space after the NUL.
> 'rnew->uri' is allocated via 'ap_escape_uri' which is defined as:
>
> #define ap_escape_uri(ppool,path) ap_os_escape_path(ppool,path,1)
>
> So, what matters here is the case in 'ap_os_escape_path' where partial *is* set.
>
> However, I have commited r1589599 in order to update the comment and to allocate one extra byte to be safe, even if not needed in this particular case.
Soon this will get replaced by the apr_escape API, which will allocate the precise amount of space for each buffer. We must make sure that code is relying on side effects of the original code.
The apr_escape API will allow you to ask for the required amount of space, which you can then manipulate before allocating the space, we must just be clear should we need to do this.
Regards,
Graham
--
Re: svn commit: r1384924 - in /httpd/httpd/trunk: include/httpd.h
server/request.c
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Hi,
the comment is wrong.
'ap_sub_req_lookup_dirent' uses the fact that 'rnew->uri' has some extra
space after the NUL.
'rnew->uri' is allocated via 'ap_escape_uri' which is defined as:
#define ap_escape_uri(ppool,path) ap_os_escape_path(ppool,path,1)
So, what matters here is the case in 'ap_os_escape_path' where partial
*is* set.
However, I have commited r1589599 in order to update the comment and to
allocate one extra byte to be safe, even if not needed in this
particular case.
Best regards,
CJ
Le 22/04/2014 11:03, Yann Ylavic a écrit :
> On Tue, Apr 22, 2014 at 9:47 AM, Christophe JAILLET
> <ch...@wanadoo.fr> wrote:
>> The first part of the comment, against 'ap_os_escape_path', is, IMO, wrong.
>> We are not guaranteed that, if partial is *not* set, that there will be one
>> byte of additional space after the NUL.
>>
>> If partial *is* set, then we skip the :
>> *d++ = '.';
>> *d++ = '/';
>> and extra bytes will be available after the NUL.
>>
>> But if it is *not* set, I think that there may be (unlikely) cases where not
>> space is available at the end.
>>
>> So, either the comment should be updated or 1 extra byte should be
>> allocated, to be safe. In this later case, the comment should also be
>> updated to state that in *all* cases, one extra byte is available.
>> +1 for allocating an extra byte.
> +1 for extra byte too, appending '/' without (re)allocating is quite useful.
>
>>
>> Moreover, 'ap_os_escape_path' could be tweaked as in r1485723.
> -0, it's a compromise between speed and space, filenames (paths) are
> often shorter than logs (and maybe less subject to escaping), I'm not
> sure it's worth the second loop in this case.
>
> Regards,
> Yann.
>
Re: svn commit: r1384924 - in /httpd/httpd/trunk: include/httpd.h server/request.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Apr 22, 2014 at 9:47 AM, Christophe JAILLET
<ch...@wanadoo.fr> wrote:
> The first part of the comment, against 'ap_os_escape_path', is, IMO, wrong.
> We are not guaranteed that, if partial is *not* set, that there will be one
> byte of additional space after the NUL.
>
> If partial *is* set, then we skip the :
> *d++ = '.';
> *d++ = '/';
> and extra bytes will be available after the NUL.
>
> But if it is *not* set, I think that there may be (unlikely) cases where not
> space is available at the end.
>
> So, either the comment should be updated or 1 extra byte should be
> allocated, to be safe. In this later case, the comment should also be
> updated to state that in *all* cases, one extra byte is available.
> +1 for allocating an extra byte.
+1 for extra byte too, appending '/' without (re)allocating is quite useful.
>
>
> Moreover, 'ap_os_escape_path' could be tweaked as in r1485723.
-0, it's a compromise between speed and space, filenames (paths) are
often shorter than logs (and maybe less subject to escaping), I'm not
sure it's worth the second loop in this case.
Regards,
Yann.
Re: svn commit: r1384924 - in /httpd/httpd/trunk: include/httpd.h
server/request.c
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Hi,
while looking at candidates for backport to synch 2.4.x and trunk, I
came across this old comment update.
The first part of the comment, against 'ap_os_escape_path', is, IMO, wrong.
We are not guaranteed that, if partial is *not* set, that there will be
one byte of additional space after the NUL.
If partial *is* set, then we skip the :
*d++ = '.';
*d++ = '/';
and extra bytes will be available after the NUL.
But if it is *not* set, I think that there may be (unlikely) cases where
not space is available at the end.
So, either the comment should be updated or 1 extra byte should be
allocated, to be safe. In this later case, the comment should also be
updated to state that in *all* cases, one extra byte is available.
+1 for allocating an extra byte.
Moreover, 'ap_os_escape_path' could be tweaked as in r1485723.
Best regards,
CJ
Le 14/09/2012 23:06, sf@apache.org a écrit :
> Author: sf
> Date: Fri Sep 14 21:06:05 2012
> New Revision: 1384924
>
> URL: http://svn.apache.org/viewvc?rev=1384924&view=rev
> Log:
> ap_sub_req_lookup_dirent() depends on the over-allocation done by
> ap_make_full_path and ap_escape_uri, so let's document it so that it is not
> accidentally removed.
>
> Modified:
> httpd/httpd/trunk/include/httpd.h
> httpd/httpd/trunk/server/request.c
>
> Modified: httpd/httpd/trunk/include/httpd.h
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/httpd.h?rev=1384924&r1=1384923&r2=1384924&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/include/httpd.h (original)
> +++ httpd/httpd/trunk/include/httpd.h Fri Sep 14 21:06:05 2012
> @@ -1579,7 +1579,9 @@ AP_DECLARE(char *) ap_escape_path_segmen
> * @param p The pool to allocate from
> * @param path The path to convert
> * @param partial if set, assume that the path will be appended to something
> - * with a '/' in it (and thus does not prefix "./")
> + * with a '/' in it (and thus does not prefix "./").
> + * If not set, there will be one byte of additional space after the
> + * NUL, to allow the caller to append a '/'.
> * @return The converted URL
> */
> AP_DECLARE(char *) ap_os_escape_path(apr_pool_t *p, const char *path, int partial);
> @@ -1692,7 +1694,8 @@ AP_DECLARE(char *) ap_make_dirstr_parent
> * @param a The pool to allocate from
> * @param dir The directory name
> * @param f The filename
> - * @return A copy of the full path
> + * @return A copy of the full path, with one byte of extra space after the NUL
> + * to allow the caller to add a trailing '/'.
> * @note Never consider using this function if you are dealing with filesystem
> * names that need to remain canonical, unless you are merging an apr_dir_read
> * path and returned filename. Otherwise, the result is not canonical.
>
> Modified: httpd/httpd/trunk/server/request.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/request.c?rev=1384924&r1=1384923&r2=1384924&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/request.c (original)
> +++ httpd/httpd/trunk/server/request.c Fri Sep 14 21:06:05 2012
> @@ -2160,7 +2160,7 @@ AP_DECLARE(request_rec *) ap_sub_req_loo
> }
>
> if (rnew->finfo.filetype == APR_DIR) {
> - /* ap_make_full_path overallocated the buffers
> + /* ap_make_full_path and ap_escape_uri overallocated the buffers
> * by one character to help us out here.
> */
> strcat(rnew->filename, "/");