Re: [PATCH] don't access(r/w) uri[-1] when validating resource w/empty uri string
Nick Kew wrote:
On Thu, 05 Jul 2012 19:33:18 +0200
Jim Meyering j...@meyering.net wrote:
Thanks for the patch, but can you clarify?
At first I thought there must be code to guarantee
that a URI (resource-uri) has length 0,
In principle it must be for an HTTP request to exist.
Have you found a testcase or code path in which it fails?
Hi Nick,
I found it via inspection.
No reproducer (if I had one, I would have posted it)
but since I found
similar guards against precisely that case, e.g.,
That could be because it's necessary, but it could also be
because the authors of those bits of code were over-cautious,
or because the code is old and extra tests were needed when
it was written. The mod_dav instance is a different URI.
it seems best to guard the use below, too:
Indeed, if there is a code path that leaves null or empty
r-uri then fixing it is right.
From 5609908643d8456c6f56197102161e56d87e56c4 Mon Sep 17 00:00:00 2001
Huh? Did you send a patch in 2001?
Sure, plenty, but not that one ;-)
I cloned from git://git.apache.org/httpd.git and posted
the output of git format-patch. That type of patch
always includes such a From line. The Date: line
is the one that matters.
uri = resource-uri;
uri_len = strlen(uri);
-if (uri[uri_len - 1] == '/') {
+if (uri_len 1 uri[uri_len - 1] == '/') {
That fixes empty uri, but segfaults on null URI.
Makes sense if the first is possible but the second isn't.
If uri is NULL, then the preceding strlen would segfault,
regardless of the proposed change. It seems pretty clear,
from all of the unprotected dereferences like that strlen,
that resource-uri cannot be NULL. However, I could not
see anything that guarantees it is not the empty string.
[PATCH] don't access(r/w) uri[-1] when validating resource w/empty uri string
At first I thought there must be code to guarantee
that a URI (resource-uri) has length 0, but since I found
similar guards against precisely that case, e.g.,
modules/dav/fs/repos.c-822
char *uri = ap_make_dirstr_parent(ctx-pool, resource-uri);
if (strlen(uri) 1 uri[strlen(uri) - 1] == '/')
uri[strlen(uri) - 1] = '\0';
modules/mappers/mod_dir.c-231
/* Redirect requests that are not '/' terminated */
if (r-uri[0] == '\0' || r-uri[strlen(r-uri) - 1] != '/')
modules/metadata/mod_cern_meta.c:293
if (r-finfo.filetype == APR_DIR || r-uri[strlen(r-uri) - 1] == '/') {
[ As I was looking through these other examples, I see that
a zero-length r-uri could cause trouble here, too, since
the above is *not* guarded. ]
it seems best to guard the use below, too:
From 5609908643d8456c6f56197102161e56d87e56c4 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Thu, 7 Jun 2012 20:36:16 +0200
Subject: [PATCH] don't access(r/w) uri[-1] when validating resource w/empty
uri string
* modules/dav/main/util.c (dav_validate_resource_state):
Handle a zero-length URI string.
---
modules/dav/main/util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/modules/dav/main/util.c b/modules/dav/main/util.c
index d076cc4..aed 100644
--- a/modules/dav/main/util.c
+++ b/modules/dav/main/util.c
@@ -984,11 +984,11 @@ static dav_error * dav_validate_resource_state(apr_pool_t
*p,
** URIs, but the majority of URIs provided to us via a resource walk
** will not contain that trailing slash.
*/
uri = resource-uri;
uri_len = strlen(uri);
-if (uri[uri_len - 1] == '/') {
+if (uri_len 1 uri[uri_len - 1] == '/') {
dav_set_bufsize(p, pbuf, uri_len);
memcpy(pbuf-buf, uri, uri_len);
pbuf-buf[--uri_len] = '\0';
uri = pbuf-buf;
}
--
1.7.11.1.116.g8228a23
Re: [PATCH] don't access(r/w) uri[-1] when validating resource w/empty uri string
On Thu, 05 Jul 2012 19:33:18 +0200
Jim Meyering j...@meyering.net wrote:
Thanks for the patch, but can you clarify?
At first I thought there must be code to guarantee
that a URI (resource-uri) has length 0,
In principle it must be for an HTTP request to exist.
Have you found a testcase or code path in which it fails?
but since I found
similar guards against precisely that case, e.g.,
That could be because it's necessary, but it could also be
because the authors of those bits of code were over-cautious,
or because the code is old and extra tests were needed when
it was written. The mod_dav instance is a different URI.
it seems best to guard the use below, too:
Indeed, if there is a code path that leaves null or empty
r-uri then fixing it is right.
From 5609908643d8456c6f56197102161e56d87e56c4 Mon Sep 17 00:00:00 2001
Huh? Did you send a patch in 2001?
uri = resource-uri;
uri_len = strlen(uri);
-if (uri[uri_len - 1] == '/') {
+if (uri_len 1 uri[uri_len - 1] == '/') {
That fixes empty uri, but segfaults on null URI.
Makes sense if the first is possible but the second isn't.
--
Nick Kew
