[PATCH v2] urlmatch.c: recompute ptr after append_normalized_escapes

["Kyle J. McKay" <mackyle@gmail.com>]

On Sep 12, 2013, at 02:57, Thomas Rast wrote:

> The calls to strbuf_add* within append_normalized_escapes() can
> reallocate the buffer passed to it.  Therefore, the seg_start pointer
> into the string cannot be kept across such calls.

Thanks for finding this.

> It went undetected for a while because it does not fail the test: the
> calls to test-urlmatch-normalization happen inside a $() substitution.
> 
> I checked the other call sites to append_normalized_escapes() for the
> same type of problem, and they seem to be okay.

> diff --git a/urlmatch.c b/urlmatch.c
> index 1db76c8..59abc80 100644
> --- a/urlmatch.c
> +++ b/urlmatch.c
> @@ -281,7 +281,8 @@ char *url_normalize(const char *url, struct url_info *out_info)
> 		url_len--;
> 	}
> 	for (;;) {
> -		const char *seg_start = norm.buf + norm.len;
> +		const char *seg_start;
> +		size_t prev_len = norm.len;

How about a more descriptive name for what prev_len is?  It's actually the
segment start offset.

> 		const char *next_slash = url + strcspn(url, "/?#");
> 		int skip_add_slash = 0;
> 		/*
> @@ -297,6 +298,7 @@ char *url_normalize(const char *url, struct url_info *out_info)
> 			strbuf_release(&norm);
> 			return NULL;
> 		}
> +		seg_start = norm.buf + prev_len;

A comment would be nice here to remind folks who might be tempted to
revert this to the previous version why it's being done this way.

I'm sure at some point someone will propose a "simplification" patch
otherwise.

Also some nits.  The patch description should be imperative mood
(cf. Documentation/SubmittingPatches).  And instead of mentioning the seg_start
pointer in the description (which will be meaningless to just about everyone and
it's clear from the diff), mention the bad thing the code was doing in more
general terms that will be clear to anyone familiar with a strbuf.

So how about this patch instead...

-- 8< --
From: Thomas Rast <trast@inf.ethz.ch>
Subject: urlmatch.c: recompute pointer after append_normalized_escapes

When append_normalized_escapes is called, its internal strbuf_add* calls can
cause the strbuf's buf to be reallocated changing the value of the buf pointer.

Do not use the strbuf buf pointer from before any append_normalized_escapes
calls afterwards.  Instead recompute the needed pointer.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---
 urlmatch.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/urlmatch.c b/urlmatch.c
index 1db76c89..01c67467 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -281,8 +281,9 @@ char *url_normalize(const char *url, struct url_info *out_info)
 		url_len--;
 	}
 	for (;;) {
-		const char *seg_start = norm.buf + norm.len;
+		const char *seg_start;
+		size_t seg_start_off = norm.len;
 		const char *next_slash = url + strcspn(url, "/?#");
 		int skip_add_slash = 0;
 		/*
 		 * RFC 3689 indicates that any . or .. segments should be
@@ -297,6 +298,8 @@ char *url_normalize(const char *url, struct url_info *out_info)
 			strbuf_release(&norm);
 			return NULL;
 		}
+		/* append_normalized_escapes can cause norm.buf to change */
+		seg_start = norm.buf + seg_start_off;
 		if (!strcmp(seg_start, ".")) {
 			/* ignore a . segment; be careful not to remove initial '/' */
 			if (seg_start == path_start + 1) {
-- 
1.8.3